New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FEAT: Add distributed filters topology #5608

Open

ramin4667 wants to merge 5 commits into main from FEAT__Add_distributed_filters

Contributor

ramin4667 commented Dec 23, 2024

Description

This feature adds attributes of the topology page of the distributed filters

Issue linked

Please mention the issue number or describe the problem this pull request addresses.

Checklist

[x ] I have tested my changes locally.
I have added necessary documentation or updated existing documentation.
[x ] I have followed the coding style guidelines of this project.
I have added appropriate tests (unit, integration, system).
[ x] I have reviewed my changes before submitting this pull request.
[x ] I have linked the issue or issues that are solved by the PR if any.
I have agreed with the Contributor License Agreement (CLA).


          FEAT: Add distributed filters

2f37b0d

Contributor

ansys-reviewer-bot bot commented Dec 23, 2024

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review


          Merge branch 'main' into FEAT__Add_distributed_filters

09ef5c9

github-actions bot added the enhancement label

codecov bot commented Dec 23, 2024 •

edited

Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.26%. Comparing base (3a67112) to head (5275ce8).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5608   +/-   ##
=======================================
  Coverage   85.26%   85.26%           
=======================================
  Files         152      152           
  Lines       61016    61016           
=======================================
+ Hits        52025    52028    +3     
+ Misses       8991     8988    -3

ramin4667 requested a review from myoung301

December 23, 2024 20:10

myoung301 requested changes

View reviewed changes

Contributor

myoung301 left a comment

The new method for creating a design is nice and clean! The tests look really thorough with testing for error messages.

Please see review comments and let me know if you have questions or it would help to go through some of it together.

src/ansys/aedt/core/filtersolutions.py

                   Examples
                   --------
                   Create a ``FilterSolutions`` instance with a band-pass elliptic ideal filter.
                   >>> import ansys.aedt.core
-                  >>> from ansys.aedt.core.filtersolutions_core.attributes import FilterImplementation
+                  >>> import ansys.aedt.core.filtersolutions
+                  >>> from ansys.aedt.core.filtersolutions_core.dll_interface import DllInterface

Contributor

myoung301 Dec 26, 2024

Is it expected that the API user will use DllInterface to get the version? I thought that DllInterface was more for internal functionality and not something the user would need to worry about.

src/ansys/aedt/core/filtersolutions.py

-                  >>> design = ansys.aedt.core.FilterSolutions(version="2025 R1", projectname= "fs1",
-                  >>> implementation_type= FilterImplementation.LUMPED,
-                  >>> )
+                  >>> design = ansys.aedt.core.FilterSolutions.LumpDesign(version= desktop_version)

Contributor

myoung301 Dec 26, 2024

If FilterSolutions is the base class for the design types and is not intended to be used directly, we probably don't need a usage example. Also, the name is misleading then too. If named FilterSolutions, it implies this is the main class for using the Filter Solutions API (as HFSS class is for HFSS API). However, it's the child classes (LumpedDesign, DistributedDesign) that the user will want. So, I think it probably makes sense to rename this class to FilterSolutionsDesignBase or something along those lines and make it clear in the class documentation that this is a parent class that shouldn't be instantiated directly and the child design classes are what should be used to create a design.

src/ansys/aedt/core/filtersolutions.py

+                  See Also
+                  --------
+                  :doc:`filtersolutions`

Contributor

myoung301 Dec 26, 2024

I'm not sure what this refers to - this file? Or is there another filtersolutions somewhere?

src/ansys/aedt/core/filtersolutions.py

-                      else:
-                          raise RuntimeError("The " + str(implementation_type) + " is not supported in this release.")
+              class LumpDesign(FilterSolutions):

Contributor

myoung301 Dec 26, 2024

Should this be LumpDesign or LumpedDesign? I think we are referring to a 'Lumped Filter', not a 'Lump Filter'.

src/ansys/aedt/core/filtersolutions.py

                       self.attributes = Attributes()
                       self.ideal_response = IdealResponse()
                       self.graph_setup = GraphSetup()
+                      self.source_impedance_table = LumpedTerminationImpedance(TerminationType.SOURCE)

Contributor

myoung301 Dec 26, 2024

Can things like source_impedance_table that are shared by all design types go into the base class init?

tests/system/general/test_45_FilterSolutions/test_lumped_filter/test_lumped_parasitics.py

@@ @@ -37,57 +37,65 @@ @@
               class TestClass:
                   def test_lumped_capacitor_q(self):
-                      lumpdesign = ansys.aedt.core.FilterSolutions(implementation_type=FilterImplementation.LUMPED)
+                      lumpdesign = ansys.aedt.core.filtersolutions.LumpDesign(config["desktopVersion"])

Contributor

myoung301 Dec 27, 2024

Use fixture.

...eneral/test_45_FilterSolutions/test_lumped_filter/test_lumped_termination_impedance_table.py

@@ @@ -36,19 +36,22 @@ @@
               @pytest.mark.skipif(config["desktopVersion"] < "2025.1", reason="Skipped on versions earlier than 2025.1")
               class TestClass:
                   def test_row_count(self):
-                      lumpdesign = ansys.aedt.core.FilterSolutions(implementation_type=FilterImplementation.LUMPED)
+                      lumpdesign = ansys.aedt.core.filtersolutions.LumpDesign(config["desktopVersion"])

Contributor

myoung301 Dec 27, 2024

Use fixture.

tests/system/general/test_45_FilterSolutions/test_raise_error.py

@@ @@ -35,7 +35,8 @@ @@
               @pytest.mark.skipif(config["desktopVersion"] < "2025.1", reason="Skipped on versions earlier than 2025.1")
               class TestClass:
                   def test_raise_error(self):
-                      design = ansys.aedt.core.FilterSolutions(implementation_type=FilterImplementation.LUMPED)
+                      design = ansys.aedt.core.filtersolutions.LumpDesign(config["desktopVersion"])

Contributor

myoung301 Dec 27, 2024

If fixtures are in common file, re-use it here.

tests/system/general/test_45_FilterSolutions/test_lumped_export/test_export_to_aedt.py

@@ @@ -49,15 +49,17 @@ @@
               @pytest.mark.skipif(config["desktopVersion"] < "2025.2", reason="Skipped on versions earlier than 2025.2")
               class TestClass:
                   def test_modelithics_inductor_list_count(self):
-                      lumpdesign = ansys.aedt.core.FilterSolutions(implementation_type=FilterImplementation.LUMPED)
+                      lumpdesign = ansys.aedt.core.filtersolutions.LumpDesign(config["desktopVersion"])

Contributor

myoung301 Dec 27, 2024

Use fixture.

tests/system/general/test_45_FilterSolutions/test_lumped_filter/test_lumped_topology.py

@@ @@ -39,50 +39,59 @@ @@
               @pytest.mark.skipif(config["desktopVersion"] < "2025.1", reason="Skipped on versions earlier than 2025.1")
               class TestClass:
                   def test_lumped_generator_resistor_30(self):
-                      lumpdesign = ansys.aedt.core.FilterSolutions(implementation_type=FilterImplementation.LUMPED)
+                      lumpdesign = ansys.aedt.core.filtersolutions.LumpDesign(config["desktopVersion"])

Contributor

myoung301 Dec 27, 2024

Use fixture.

ramin4667 added 3 commits

December 28, 2024 18:07


          FAET: Filter implementation is removed

da0174d


          Merge remote-tracking branch 'origin/main' into FEAT__Add_distributed…

73ebce0

…_filters


          Merge branch 'FEAT__Add_distributed_filters' of https://github.com/an…

5275ce8

…sys/pyaedt into FEAT__Add_distributed_filters

# Conflicts:
#	src/ansys/aedt/core/filtersolutions.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels