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 87.71930% with 7 lines in your changes missing coverage. Please review.

Project coverage is 84.39%. Comparing base (f73c6c9) to head (8b94bd1).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5283   +/-   ##
=======================================
  Coverage   84.39%   84.39%           
=======================================
  Files         140      140           
  Lines       58645    58684   +39     
=======================================
+ Hits        49493    49528   +35     
- Misses       9152     9156    +4

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

…ges. All elements now support names that are Strings, Integers, Floats or left blank as per expected Maxwell circuit schematic usage.

SMoraisAnsys · 2024-10-21T14:47:49Z

I went through Circuit help PDF files and you might find something usefull with SelectPage and GetNumPages.

DaveTwyman · 2024-10-21T15:19:00Z

I went through Circuit help PDF files and you might find something usefull with SelectPage and GetNumPages.

Yes!, 'GetNumPages', how did I miss that, Thanks

SMoraisAnsys · 2024-10-29T10:45:19Z

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

DaveTwyman · 2024-10-29T11:09:01Z

Yes, I'm planning to get back to finish that shortly

…

________________________________ From: Sébastien Morais ***@***.***> Sent: Tuesday, October 29, 2024 10:45 To: ansys/pyaedt ***@***.***> Cc: David Twyman ***@***.***>; Mention ***@***.***> Subject: Re: [ansys/pyaedt] FEAT: VSin, ISin Sources added to Maxwell Circuit Primitives (PR #5283) [External Sender] Hi @DaveTwyman<https://github.com/DaveTwyman>, could you please add the last changes required for this PR ? :) — Reply to this email directly, view it on GitHub<#5283 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AVVIYTCE36KV4RDR3OZAJQDZ55RNJAVCNFSM6AAAAABPYVZE36VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBTHA3DMNBZGQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

… 53_get_num_pages

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 <[email protected]>

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.

SMoraisAnsys · 2024-11-14T07:53:48Z

src/ansys/aedt/core/maxwellcircuit.py

+        self.oeditor.CreatePage(name)
+        return True
+
+    def get_num_pages(self):


I think it would be better to add nb_pages as a property instead of adding this feature as a method.

@SMoraisAnsys , That would be a better implementation. I'll make that switch to a parameter

Hello @SMoraisAnsys
Can I double check my planned implementation is the same as your suggestion.
This change would mean 'get_num_pages' would be accessed as an attribute of the MaxwellCircuit class?

SMoraisAnsys · 2024-11-14T07:54:10Z

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

+        self.oeditor.CreatePage(name)
+        return True
+
+    def get_num_pages(self):


I think it would be better to add nb_pages as a property instead of adding this feature as a method.

SMoraisAnsys · 2024-11-14T07:58:31Z

tests/system/general/test_35_MaxwellCircuit.py

+        assert self.aedtapp.create_page(3.14) == True
+        assert not self.aedtapp.create_page(["create_page_test"])
+
+    def test_12_get_num_pages(self):


Could you please add a test on the return value and not only on the value type ?

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

DaveTwyman and others added 2 commits October 18, 2024 12:10

Added support for Name=None case which previously created error messa…

359f752

…ges. All elements now support names that are Strings, Integers, Floats or left blank as per expected Maxwell circuit schematic usage.

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

fdf3786

DaveTwyman and others added 2 commits October 30, 2024 20:35

Added method 'get_num_pages' as well as unit tests 52_create_page and…

4ef9e9f

… 53_get_num_pages

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 <[email protected]>

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

a4dd34f

Co-authored-by: gmalinve <[email protected]>

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

7aa718d

Co-authored-by: gmalinve <[email protected]>

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

bd322c4

Co-authored-by: gmalinve <[email protected]>

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

44827f3

Co-authored-by: gmalinve <[email protected]>

gmalinve reviewed Nov 5, 2024

View reviewed changes

src/ansys/aedt/core/modeler/circuits/primitives_circuit.py 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

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

SMoraisAnsys commented Oct 21, 2024

DaveTwyman commented Oct 21, 2024

SMoraisAnsys commented Oct 29, 2024

DaveTwyman commented Oct 29, 2024 via email

DaveTwyman commented Oct 31, 2024

SMoraisAnsys commented Oct 31, 2024

gmalinve left a comment

SMoraisAnsys left a comment

SMoraisAnsys Nov 14, 2024

DaveTwyman Nov 14, 2024

DaveTwyman Nov 14, 2024

SMoraisAnsys Nov 14, 2024

SMoraisAnsys Nov 14, 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

SMoraisAnsys commented Oct 21, 2024

DaveTwyman commented Oct 21, 2024

SMoraisAnsys commented Oct 29, 2024

DaveTwyman commented Oct 29, 2024 via email

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 Nov 14, 2024

Choose a reason for hiding this comment

DaveTwyman Nov 14, 2024

Choose a reason for hiding this comment

DaveTwyman Nov 14, 2024

Choose a reason for hiding this comment

SMoraisAnsys Nov 14, 2024

Choose a reason for hiding this comment

SMoraisAnsys Nov 14, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 11, 2024 •

edited

Loading

gmalinve commented Oct 14, 2024 •

edited

Loading