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: Filtersolutions #4858

Merged
merged 38 commits into from
Jul 17, 2024
Merged

FEAT: Filtersolutions #4858

merged 38 commits into from
Jul 17, 2024

Conversation

ramin4667
Copy link
Contributor

This feature adds the Filtersolutions Python application to the PyAEDT.

ramin4667 and others added 26 commits May 15, 2024 11:40
@ramin4667 ramin4667 requested a review from myoung301 June 28, 2024 11:29
@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 examples Anything related to the examples label Jun 28, 2024
@ramin4667 ramin4667 requested a review from SMoraisAnsys July 3, 2024 15:32
@ramin4667
Copy link
Contributor Author

Reviewer comments are addressed and all tests are passed locally except the version comparison, pending a DLL update.

FAILED _unittest/test_45_FilterSolutions/test_filter/test_dll_interface.py::TestClass::test_version - AssertionError: assert 'FilterSolutions API Version 2024 R1 (Beta)' == 'FilterSolutions API Version 2025 R1'

  - FilterSolutions API Version 2025 R1
  ?                                ^
  + FilterSolutions API Version 2024 R1 (Beta)
  ?                                ^   +++++++``

Also, the code coverage is tested locally with the following report:

> pyaedt\filtersolutions.py                                               33      1    97%   74
> pyaedt\filtersolutions_core\__init__.py                                 12      1    92%   41
> pyaedt\filtersolutions_core\attributes.py                              777      3    99%   804-805, 809
> pyaedt\filtersolutions_core\dll_interface.py                            64      3    95%   52, 56, 60
> pyaedt\filtersolutions_core\graph_setup.py                              53      0   100%
> pyaedt\filtersolutions_core\ideal_response.py                          165      0   100%
> pyaedt\filtersolutions_core\lumped_nodes_and_leads.py                  106      0   100%
> pyaedt\filtersolutions_core\lumped_parasitics.py                        97      0   100%
> pyaedt\filtersolutions_core\lumped_termination_impedance.py            142      0   100%
> pyaedt\filtersolutions_core\lumped_topology.py                         310      0   100%
> pyaedt\filtersolutions_core\multiple_bands_table.py                     56      0   100%
> pyaedt\filtersolutions_core\transmission_zeros.py                       76      0   100%

@myoung301
Copy link
Contributor

Hi @SMoraisAnsys, I see this is pending your review. Is there anything else that needs to be changed with this PR before it's ready to approve and merge?

maxcapodi78
maxcapodi78 previously approved these changes Jul 16, 2024
Copy link
Collaborator

@maxcapodi78 maxcapodi78 left a comment

Choose a reason for hiding this comment

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

Is there any specific reason why the tests don't run in version lower than 25R1?
In such case please add filtersolutions.py and filtersolutions_core to the exclusion list in codecov.yml file and merge.
Please make sure, in next PR to add documentation rst to generate automatically documentation for it.

@myoung301
Copy link
Contributor

@maxcapodi78 Thank you for the review. The FS API is available starting with 2025 R1 so the tests won't run unless that version or later is used. We've added the FilterSolutions-specific files and folders to the exclusion list. There is one common file that had a few lines added by this PR that doesn't pass the coverage check because the new lines aren't tested until 2025 R1. I think this is ok, but if you'd like us to add # pragma: no cover, let us know and we'll get that in a follow-up PR. We'll also look to address the documentation in another PR soon.

@myoung301 myoung301 dismissed SMoraisAnsys’s stale review July 17, 2024 16:26

All comments were addressed and maxcapodi78 provided an updated review.

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.

Comments were addressed and maxcapodi78 approved. Approval was reset when coverage exclusion updates were pushed so re-approving now to merge.

@myoung301 myoung301 merged commit cddb301 into main Jul 17, 2024
45 of 46 checks passed
@myoung301 myoung301 deleted the FEAT_Filtersolutions branch July 17, 2024 16:28
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 examples Anything related to the examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants