FEAT: VSin, ISin Sources added to Maxwell Circuit Primitives #5283

DaveTwyman · 2024-10-11T10:59:40Z

Create Page and Select Page added to Circuit Primitives

ansys-reviewer-bot · 2024-10-11T10:59:43Z

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

@ansys-reviewer-bot review

for more information, see https://pre-commit.ci

codecov · 2024-10-11T11:30:28Z

Codecov Report

Attention: Patch coverage is 89.47368% with 6 lines in your changes missing coverage. Please review.

Project coverage is 83.32%. Comparing base (8717418) to head (9f129ad).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5283      +/-   ##
==========================================
- Coverage   84.59%   83.32%   -1.28%     
==========================================
  Files         142      142              
  Lines       59941    59980      +39     
==========================================
- Hits        50710    49980     -730     
- Misses       9231    10000     +769

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

Create Page and Select Page added to Circuit Primitives

…s' into 5282-Add-Missing-Circuit-Elements # Conflicts: # src/ansys/aedt/core/modeler/circuits/primitives_circuit.py

Create Page and Select Page added to Circuit Primitives

for more information, see https://pre-commit.ci

SMoraisAnsys

Thanks for the changes, could you address the comments and add tests ?

src/ansys/aedt/core/modeler/circuits/primitives_maxwell_circuit.py

Create Page added to Circuit Primitives

for more information, see https://pre-commit.ci

gmalinve · 2024-10-14T10:35:11Z

@DaveTwyman please install pre-commit in your venv because I see that every time you push a commit it automatically triggers the auto fixes pre-commit:

This package will automatically run all the checks specified in .pre-commit-config.yaml when you push a commit.

Also remember to update and merge main in your branch from time to time.

DaveTwyman · 2024-10-14T10:44:25Z

@DaveTwyman please install pre-commit in your venv because I see that every time you push a commit it automatically triggers the auto fixes pre-commit: This package will automatically run all the checks specified in .pre-commit-config.yaml when you push a commit.

Also remember to update and merge main in your branch from time to time.

@gmalinve Good point about the pre-commit, I see it's installed on my machine. I've just updated it too. I guess the issue is I need to start using it before each push.

I believe I'm updating main before each push from PyCharm, but let me know if you can see evidence something is wrong here. Thanks again as always

SMoraisAnsys · 2024-10-14T10:59:20Z

@DaveTwyman If it is installed on your machine then it shouldn't let you commit. The precommit should trigger the problems locally before allowing you to make any commit.

DaveTwyman · 2024-10-14T11:08:34Z

@DaveTwyman If it is installed on your machine then it shouldn't let you commit. The precommit should trigger the problems locally before allowing you to make any commit.

Ok, understood, I'm just reading up on how to set this up now

gmalinve · 2024-10-16T14:29:54Z

@DaveTwyman you need to add unit tests now :)

DaveTwyman · 2024-10-31T09:17:15Z

Hi @DaveTwyman, could you please add the last changes required for this PR ? :)

Hello @SMoraisAnsys , I've added the last unit test, and method needed to support that test (plus another unit test for the new method). So all the new methods now have supporting unit tests.

It seems that the test coverage codecov is very low, do you think this is because of making multiple single line changes 'id' to 'component' or issues elsewhere?

SMoraisAnsys · 2024-10-31T09:27:20Z

Try to run your test locally and see if it is correctly covered. It might be a codecov issue (but I doubt about it).
It might be because you are not really going through the lines you expect to go through.
I can't have a look at your PR now and giving general advice so that you can check where the coverage issue comes from.

gmalinve

@DaveTwyman please review and fix all the comments.
You created unit tests that don't run at all. Please test, run and debug locally before pushing commits to your PR.

_unittest/test_21_Circuit.py

_unittest/test_35_MaxwellCircuit.py

src/ansys/aedt/core/modeler/circuits/primitives_circuit.py

src/ansys/aedt/core/modeler/circuits/primitives_maxwell_circuit.py

Co-authored-by: gmalinve <103059376+gmalinve@users.noreply.github.com>

src/ansys/aedt/core/modeler/circuits/primitives_circuit.py

…uld not be in circuit.py. -Replaced set_property method with parameter method. -Added example in docstring for create_i_sin and create_v_sin. -Added more checks for create_voltage_source and create_current_source name types in unit tests. -Moved create_page and get_num_pages unit tests to test_35_MaxwellCircuit.py -Added assert not to two of the unit tests

…Missing-Circuit-Elements

# Conflicts: # tests/system/general/test_21_Circuit.py

For more information, see https://pre-commit.ci

SMoraisAnsys

I think it would be better to add the get number pages feature as a property, could you update the code ?
Also, the added code in primitives_circuit.py is not tested, could you test it ?
I left an extra minor comment.

src/ansys/aedt/core/maxwellcircuit.py

src/ansys/aedt/core/modeler/circuits/primitives_circuit.py

tests/system/general/test_35_MaxwellCircuit.py

Unit test 12 no longer needed. Added nb_pages after each page is created in unit test 11 to catch cases where pages are not created.

…ed 'nb_pages'. Change made for circuit so implementation is the same as maxwellcircuit.

SMoraisAnsys · 2024-11-21T09:58:05Z

Hey @DaveTwyman, I'm not sure about the status of this PR. Do you want to add VSin and ISin on top of the pages features or should this be splited into two PRs and some commits got added inadvertently ?
About the pages features, can you add a test to create_page on CircuitComponents class ? Also, would it make sense to add nb_pages to this class ?

VSin, ISin Sources added to Maxwell Circuit Primitives

5aac760

Create Page and Select Page added to Circuit Primitives

DaveTwyman requested review from Samuelopez-ansys and gmalinve October 11, 2024 10:59

DaveTwyman and others added 2 commits October 11, 2024 12:01

Merge branch 'main' into 5282-Add-Missing-Circuit-Elements

f8699a5

[pre-commit.ci] auto fixes from pre-commit.com hooks

8552947

for more information, see https://pre-commit.ci

DaveTwyman changed the title ~~VSin, ISin Sources added to Maxwell Circuit Primitives~~ FEAT: VSin, ISin Sources added to Maxwell Circuit Primitives Oct 11, 2024

DaveTwyman and others added 4 commits October 11, 2024 13:04

VSin, ISin Sources added to Maxwell Circuit Primitives

93375bd

Create Page and Select Page added to Circuit Primitives

Merge remote-tracking branch 'origin/5282-Add-Missing-Circuit-Element…

955bcf7

…s' into 5282-Add-Missing-Circuit-Elements # Conflicts: # src/ansys/aedt/core/modeler/circuits/primitives_circuit.py

VSin, ISin Sources added to Maxwell Circuit Primitives

cf6153e

Create Page and Select Page added to Circuit Primitives

[pre-commit.ci] auto fixes from pre-commit.com hooks

eb4c79e

for more information, see https://pre-commit.ci

gmalinve marked this pull request as draft October 11, 2024 12:53

SMoraisAnsys reviewed Oct 14, 2024

View reviewed changes

DaveTwyman and others added 2 commits October 14, 2024 11:22

vsin, isin Sources added to Maxwell Circuit Primitives

a07645f

Create Page added to Circuit Primitives

[pre-commit.ci] auto fixes from pre-commit.com hooks

84e4f7a

for more information, see https://pre-commit.ci

DaveTwyman marked this pull request as ready for review October 14, 2024 10:47

Merge branch 'main' into 5282-Add-Missing-Circuit-Elements

d7945da

DaveTwyman added 3 commits October 14, 2024 13:56

Merge branch 'main' into 5282-Add-Missing-Circuit-Elements

a3a2340

Merge branch 'main' into 5282-Add-Missing-Circuit-Elements

46152e2

Merge branch 'main' into 5282-Add-Missing-Circuit-Elements

bf84832

gmalinve marked this pull request as draft October 16, 2024 15:00

SMoraisAnsys and others added 2 commits October 17, 2024 08:14

Merge branch 'main' into 5282-Add-Missing-Circuit-Elements

0c4d246

Unit Tests added for vsin and isin

d180ad8

DaveTwyman marked this pull request as ready for review October 17, 2024 09:57

Merge branch 'main' into 5282-Add-Missing-Circuit-Elements

d44fd5d

DaveTwyman marked this pull request as ready for review October 30, 2024 21:03

gmalinve requested changes Nov 5, 2024

View reviewed changes

DaveTwyman and others added 5 commits November 5, 2024 10:02

Update src/ansys/aedt/core/modeler/circuits/primitives_circuit.py

0a98ed1

Co-authored-by: gmalinve <103059376+gmalinve@users.noreply.github.com>

Update src/ansys/aedt/core/modeler/circuits/primitives_circuit.py

a4dd34f

Co-authored-by: gmalinve <103059376+gmalinve@users.noreply.github.com>

Update src/ansys/aedt/core/modeler/circuits/primitives_circuit.py

7aa718d

Co-authored-by: gmalinve <103059376+gmalinve@users.noreply.github.com>

Update src/ansys/aedt/core/modeler/circuits/primitives_circuit.py

bd322c4

Co-authored-by: gmalinve <103059376+gmalinve@users.noreply.github.com>

Update src/ansys/aedt/core/modeler/circuits/primitives_circuit.py

44827f3

Co-authored-by: gmalinve <103059376+gmalinve@users.noreply.github.com>

gmalinve reviewed Nov 5, 2024

View reviewed changes

src/ansys/aedt/core/modeler/circuits/primitives_circuit.py Outdated Show resolved Hide resolved

DaveTwyman and others added 5 commits November 12, 2024 11:25

Merge branch 'main' of https://github.com/ansys/pyaedt into 5282-Add-…

3f56946

…Missing-Circuit-Elements

Merge branch 'main' into 5282-Add-Missing-Circuit-Elements

e053aab

# Conflicts: # tests/system/general/test_21_Circuit.py

CHORE: Auto fixes from pre-commit.com hooks

8cefb93

For more information, see https://pre-commit.ci

Merge branch 'main' into 5282-Add-Missing-Circuit-Elements

44c9258

DaveTwyman requested review from SMoraisAnsys and gmalinve November 12, 2024 16:52

Merge branch 'main' into 5282-Add-Missing-Circuit-Elements

8b94bd1

DaveTwyman enabled auto-merge (squash) November 13, 2024 09:04

DaveTwyman self-assigned this Nov 13, 2024

SMoraisAnsys disabled auto-merge November 14, 2024 07:49

SMoraisAnsys requested changes Nov 14, 2024

View reviewed changes

src/ansys/aedt/core/maxwellcircuit.py Outdated Show resolved Hide resolved

src/ansys/aedt/core/modeler/circuits/primitives_circuit.py Outdated Show resolved Hide resolved

tests/system/general/test_35_MaxwellCircuit.py Outdated Show resolved Hide resolved

DaveTwyman and others added 5 commits November 19, 2024 15:45

Added property to get number of pages in schematic instead of a method.

0291fd4

Unit test 12 no longer needed. Added nb_pages after each page is created in unit test 11 to catch cases where pages are not created.

Merge branch 'main' into 5282-Add-Missing-Circuit-Elements

9496014

Merge branch 'main' into 5282-Add-Missing-Circuit-Elements

8a765fb

Changed from using a method called 'get_num_pages' to a property call…

e8bfc8f

…ed 'nb_pages'. Change made for circuit so implementation is the same as maxwellcircuit.

Merge branch 'main' into 5282-Add-Missing-Circuit-Elements

9f129ad

DaveTwyman requested a review from SMoraisAnsys November 20, 2024 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT: VSin, ISin Sources added to Maxwell Circuit Primitives #5283

FEAT: VSin, ISin Sources added to Maxwell Circuit Primitives #5283

DaveTwyman commented Oct 11, 2024

ansys-reviewer-bot bot commented Oct 11, 2024

codecov bot commented Oct 11, 2024 •

edited

Loading

SMoraisAnsys left a comment

gmalinve commented Oct 14, 2024 •

edited

Loading

DaveTwyman commented Oct 14, 2024

SMoraisAnsys commented Oct 14, 2024

DaveTwyman commented Oct 14, 2024

gmalinve commented Oct 16, 2024

DaveTwyman commented Oct 31, 2024

SMoraisAnsys commented Oct 31, 2024

gmalinve left a comment

SMoraisAnsys left a comment

SMoraisAnsys commented Nov 21, 2024

FEAT: VSin, ISin Sources added to Maxwell Circuit Primitives #5283

Are you sure you want to change the base?

FEAT: VSin, ISin Sources added to Maxwell Circuit Primitives #5283

Conversation

DaveTwyman commented Oct 11, 2024

ansys-reviewer-bot bot commented Oct 11, 2024

codecov bot commented Oct 11, 2024 • edited Loading

Codecov Report

SMoraisAnsys left a comment

Choose a reason for hiding this comment

gmalinve commented Oct 14, 2024 • edited Loading

DaveTwyman commented Oct 14, 2024

SMoraisAnsys commented Oct 14, 2024

DaveTwyman commented Oct 14, 2024

gmalinve commented Oct 16, 2024

DaveTwyman commented Oct 31, 2024

SMoraisAnsys commented Oct 31, 2024

gmalinve left a comment

Choose a reason for hiding this comment

SMoraisAnsys left a comment

Choose a reason for hiding this comment

SMoraisAnsys commented Nov 21, 2024

codecov bot commented Oct 11, 2024 •

edited

Loading

gmalinve commented Oct 14, 2024 •

edited

Loading