Skip to content
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.

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

FEAT: Add distributed filters topology #5608

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

ramin4667
Copy link
Contributor

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).

@ansys-reviewer-bot
Copy link
Contributor

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

@ansys-reviewer-bot review

@github-actions github-actions bot added the enhancement New features or code improvements label Dec 23, 2024
Copy link

codecov bot commented Dec 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.25%. Comparing base (a5cbc5e) to head (459a62c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5608   +/-   ##
=======================================
  Coverage   85.25%   85.25%           
=======================================
  Files         152      152           
  Lines       61012    61012           
=======================================
  Hits        52015    52015           
  Misses       8997     8997           

@ramin4667 ramin4667 requested a review from myoung301 December 23, 2024 20:10
Copy link
Contributor

@myoung301 myoung301 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/ansys/aedt/core/filtersolutions.py Outdated Show resolved Hide resolved

See Also
--------
:doc:`filtersolutions`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will make help later and check the help page.

src/ansys/aedt/core/filtersolutions.py Outdated Show resolved Hide resolved
src/ansys/aedt/core/filtersolutions.py Show resolved Hide resolved
@pre-commit-ci pre-commit-ci bot requested a review from jsalant22 as a code owner January 2, 2025 12:23
@github-actions github-actions bot added the testing Anything related to testing label Jan 2, 2025
@SMoraisAnsys
Copy link
Collaborator

I checked and most of the discussions have been tagged as "resolved" but nothing has been done to follow the associated comments. @ramin4667 could you please have another look at @myoung301 comments ?

@ramin4667
Copy link
Contributor Author

The code test coverage is pasted below:

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

---------- coverage: platform win32, python 3.11.8-final-0 -----------
Name Stmts Miss Cover Missing

src\ansys\aedt\core\filtersolutions_core_init_.py 12 1 92% 41
src\ansys\aedt\core\filtersolutions_core\attributes.py 880 41 95% 840-841, 845, 943-944, 948, 959-960, 964, 975-978, 982, 996-999, 1003, 1017-1018, 1022, 1034-1035, 1039, 1137-1140, 1144-1145, 1155-1158, 1162-1163, 1174-1179, 1183-1184
src\ansys\aedt\core\filtersolutions_core\distributed_topology.py 484 2 99% 283-284
src\ansys\aedt\core\filtersolutions_core\dll_interface.py 66 10 85% 52-65
src\ansys\aedt\core\filtersolutions_core\export_to_aedt.py 863 21 98% 837-839, 1222-1223, 1227-1228, 1277, 1311, 1344, 1473-1474, 1484-1487, 1491-1492, 1619, 1653-1654
src\ansys\aedt\core\filtersolutions_core\graph_setup.py 53 0 100%
src\ansys\aedt\core\filtersolutions_core\ideal_response.py 171 0 100%
src\ansys\aedt\core\filtersolutions_core\lumped_nodes_and_leads.py 106 0 100%
src\ansys\aedt\core\filtersolutions_core\lumped_parasitics.py 97 0 100%
src\ansys\aedt\core\filtersolutions_core\lumped_termination_impedance_table.py 142 0 100%
src\ansys\aedt\core\filtersolutions_core\lumped_topology.py 314 0 100%
src\ansys\aedt\core\filtersolutions_core\multiple_bands_table.py 59 0 100%
src\ansys\aedt\core\filtersolutions_core\optimization_goals_table.py 90 18 80% 307-314, 324-333, 342-343
src\ansys\aedt\core\filtersolutions_core\transmission_zeros.py 76 0 100%

TOTAL 3413 93 97%

Copy link
Collaborator

@SMoraisAnsys SMoraisAnsys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but could you add a function for the text comparison ?

It seems like a lot of test leverage a string of the form "The YOUNAMEIT property is not applicable for the OTHERNAME topology". If you add a function with two arguments to create that string, it would greatly lower the number of LOC and also simplify future refactoring if any.

If you expect the text to be very specific for each test in the near future then ignore this comment. However, from the previous state and your proposed changes, I'd advise to add the function.

@ramin4667
Copy link
Contributor Author

@SMoraisAnsys, please look at this snippet and let me know so I can update the rest.

def assert_runtime_error(distributed_design, property_name, property_condition, expected_message):
"""
Assert that setting a property on the distributed_design raises a RuntimeError with the expected message.
Parameters
----------
distributed_design: object
The distributed design object.
property_name: str
The name of the property to set.
property_condition: bool
The condition to set the property to.
expected_message: str
The expected error message.
"""
with pytest.raises(RuntimeError) as info:
setattr(distributed_design.topology, property_name, property_condition)
assert info.value.args[0] == "The " + expected_message + " property is not applicable for the "
+ convert_string(distributed_design.topology.topology_type.name) + " topology"

def test_distributed_first_shunt(self, distributed_design):
assert distributed_design.topology.first_shunt
distributed_design.topology.topology_type = TopologyType.SPACED_STUBS
assert_runtime_error(distributed_design, "first_shunt", False, "First Element Shunt or Series")
# with pytest.raises(RuntimeError) as info:
# distributed_design.topology.topology_type = TopologyType.SPACED_STUBS
# distributed_design.topology.first_shunt = False
# assert (
# info.value.args[0]
# == "The First Element Shunt or Series property is not applicable for the "
# + convert_string(distributed_design.topology.topology_type.name)
# + " topology"
# )

@ramin4667 ramin4667 closed this Jan 10, 2025
@ramin4667 ramin4667 deleted the FEAT__Add_distributed_filters branch January 10, 2025 14:44
@ramin4667 ramin4667 restored the FEAT__Add_distributed_filters branch January 10, 2025 18:36
@ramin4667 ramin4667 reopened this Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features or code improvements testing Anything related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants