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 #5687

Merged
merged 37 commits into from
Feb 3, 2025

Conversation

ramin4667
Copy link
Contributor

@ramin4667 ramin4667 commented Jan 22, 2025

Description

This PR adds attributes of the topology page of distributed filters to the FilterSolutions API.

Issue linked

All parameters, attributes, test cases, docstrings, and Sphinx help files have been addressed.

Checklist

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

This PR is a replacement for #5608 after we switched branches. A fuller review and the fixes were first applied there.

@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

@ramin4667 ramin4667 changed the title Feat add distributed filters topology FEAT: Add distributed filters topology Jan 22, 2025
@github-actions github-actions bot added testing Anything related to testing enhancement New features or code improvements labels Jan 22, 2025
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.20%. Comparing base (3c863dc) to head (8f34ac0).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5687      +/-   ##
==========================================
- Coverage   85.20%   85.20%   -0.01%     
==========================================
  Files         156      156              
  Lines       61070    61070              
==========================================
- Hits        52037    52033       -4     
- Misses       9033     9037       +4     

@ramin4667
Copy link
Contributor Author

Test coverage results are attached:

---------- 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% 845-846, 850, 951-952, 956, 968-969, 973, 985-988, 992, 1006-1009, 1013, 1027-1028, 1032, 1044-1045, 1049, 1150-1153, 1157-1158, 1168-1171, 1175-1176, 1188-1193, 1197-1198
src\ansys\aedt\core\filtersolutions_core\distributed_topology.py 478 0 100%
src\ansys\aedt\core\filtersolutions_core\dll_interface.py 67 10 85% 53-66
src\ansys\aedt\core\filtersolutions_core\export_to_aedt.py 863 21 98% 833-835, 1224-1225, 1229-1230, 1281, 1316, 1350, 1483-1484, 1494-1497, 1501-1502, 1631, 1665-1666
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% 309-316, 326-335, 344-345
src\ansys\aedt\core\filtersolutions_core\transmission_zeros.py 76 0 100%

TOTAL 3408 91 97%

@myoung301
Copy link
Contributor

Looking at the coverage results, there are some lines that aren't covered. Are there some tests missing or what did the coverage check reveal?

@ramin4667
Copy link
Contributor Author

Improved test coverage:

---------- 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 0 100%
src\ansys\aedt\core\filtersolutions_core\distributed_topology.py 478 0 100%
src\ansys\aedt\core\filtersolutions_core\dll_interface.py 67 10 85% 53-66
src\ansys\aedt\core\filtersolutions_core\export_to_aedt.py 863 8 99% 835, 1224-1225, 1229-1230, 1350, 1483-1484
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% 309-316, 326-335, 344-345
src\ansys\aedt\core\filtersolutions_core\transmission_zeros.py 76 0 100%

TOTAL 3408 37 99%

================================================================================ 264 passed, 2 warnings in 505.63s (0:08:25) =================================================================================

@myoung301 myoung301 requested a review from DulceRiosK January 27, 2025 20:37
@DulceRiosK
Copy link
Contributor

DulceRiosK commented Jan 28, 2025

Hi @ramin4667, I looked at the files changed in doc/source/API, here are my notes.

doc/source/API/FilterSolutions.rst
Line 6 remove comma after "FilterDesignBase" class
Line 17 remove extra space after the word "attributes"
Lines 19, 99, change "entries of" to "entries in the" (to match lines 100 and 102)
Line 42 change "LempedDesign" to "LumpedDesign"
Line 95 remove "s" after the word "define"

Thank you,
Dulce

doc/source/API/FilterSolutions.rst Outdated Show resolved Hide resolved
doc/source/API/FilterSolutions.rst Outdated Show resolved Hide resolved
doc/source/API/FilterSolutions.rst Outdated Show resolved Hide resolved
doc/source/API/FilterSolutions.rst Outdated Show resolved Hide resolved
doc/source/API/FilterSolutions.rst Outdated Show resolved Hide resolved
doc/source/API/FilterSolutions.rst Outdated Show resolved Hide resolved
@ramin4667
Copy link
Contributor Author

@DulceRiosK Thanks for your comments. Can you please resolve the fixed comments?

@Samuelopez-ansys
Copy link
Member

Hi @ramin4667, is this PR ready to be merged?

Please consider that if a Pull Request is not draft, it will consume the CICD resources, which will block other PRs that are ready to be merged. If you are testing something, I suggest you do it first locally and move the PR to draft, which will not trigger the unit tests and then it will allow other PRs to go ahead.

If your PR is ready, and you are just facing issues in the CICD, then nevermind, we all have these issues :)

@ramin4667
Copy link
Contributor Author

Hi @ramin4667, is this PR ready to be merged?

Please consider that if a Pull Request is not draft, it will consume the CICD resources, which will block other PRs that are ready to be merged. If you are testing something, I suggest you do it first locally and move the PR to draft, which will not trigger the unit tests and then it will allow other PRs to go ahead.

If your PR is ready, and you are just facing issues in the CICD, then nevermind, we all have these issues :)

Hi @Samuelopez-ansys, Thanks for letting me know.
This PR is almost finished and ready to merge. Only one issue is unsolved that you may help me fix.
The Codacy reports "Possible hardcoded password" for test lines including "pass" term in 17 lines like "pass_band" etc., is there a way to ignore this error?

@Samuelopez-ansys
Copy link
Member

@SMoraisAnsys Do you know why Codacy is detecting this as a password?

@ramin4667
Copy link
Contributor Author

@SMoraisAnsys Do you know why Codacy is detecting this as a password?

@Samuelopez-ansys I guess because of the term "pass".

@ramin4667
Copy link
Contributor Author

@DulceRiosK Could you please approve this PR?

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.

Looks good to me.

@myoung301 myoung301 merged commit be8f391 into main Feb 3, 2025
42 checks passed
@myoung301 myoung301 deleted the FEAT__Add_distributed_filters_topology branch February 3, 2025 21:25
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.

4 participants