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

Project coverage is 40.52%. Comparing base (25c9ca1) to head (d44fd5d).

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #5283       +/-   ##
===========================================
- Coverage   84.33%   40.52%   -43.82%     
===========================================
  Files         140      140               
  Lines       58460    58490       +30     
===========================================
- Hits        49305    23701    -25604     
- Misses       9155    34789    +25634

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

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

gmalinve · 2024-10-17T11:46:55Z

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

+            use_instance_id_netlist=use_instance_id_netlist,
+        )
+
+        id.set_property("Ia", value)


@DaveTwyman this is not correct, please debug and see what happens when you set a property that doesn't exist. You access "Ia" through parameters. you don't have to use set_property.

Hi @gmalinve, I just double checked and this works, I couldn't get a bug to appear.
Ia is one of the ten parameters that ISin can take and the most used parameter (it's the amplitude of the sinusoidal current), it's fed into PyAEDT 'create_isin' using the 'value' parameter. I did think of renaming 'value' to something like 'IPeak', 'Current' etc but since the rest of the Maxwell circuit components in the library have a 'value' parameter I've stuck with using 'value' for consistency. The usage of '.set_property' is throughout the rest of the 'primitives_maxwell_circuit' .
Were you suggesting another approach to 'set_property' that has some advantages?

@DaveTwyman this is not a bug. You simply have to access the property and change its value through the "parameters" property, without using the set_property method.

Hi @gmalinve , Sorry I'm not following this, could you tell me what you would replace line 326 with and why?
All the other circuit components in the library use this "set_property" approach

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

gmalinve · 2024-10-17T11:57:09Z

@DaveTwyman also please link your issue to this PR so once this is closed they both will be.

Renamed create_vsin and create_isin to create_v_sin and create_i_sin

SMoraisAnsys

Could you please use component as variable name instead of comp_name ? This could be confusing for future maintenance as it gives the impression that the variable is a string when it is not.
Also can you add a test for create_page ?

Thanks again for taking the time to perform all the requested changes.

DaveTwyman · 2024-10-18T10:00:45Z

Could you please use component as variable name instead of comp_name ? This could be confusing for future maintenance as it gives the impression that the variable is a string when it is not. Also can you add a test for create_page ?

Thanks again for taking the time to perform all the requested changes.

@SMoraisAnsys , Not at all, thanks for your patience whilst I work through this.

I could do with some guidance on the unit test for the 'create_page', there is no way to check on the pages name, index or other properties, the underlying IronPython doesn't have such a feature. Are you aware of another instance in PyAEDT where we create something that cannot then be queried. I could then copy that UT. In this case there isn't a useful check that can be added to the UT.

Should we do something like add a dummy UT, that just holds a comment?

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

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

gmalinve marked this pull request as draft October 17, 2024 11:33

gmalinve requested changes Oct 17, 2024

View reviewed changes

DaveTwyman linked an issue Oct 17, 2024 that may be closed by this pull request

Add Missing Circuit Elements Needed for ETK #5282

Open

Replaced id with comp_name

5a95e4e

Renamed create_vsin and create_isin to create_v_sin and create_i_sin

SMoraisAnsys requested changes Oct 18, 2024

View reviewed changes

DaveTwyman added 2 commits October 18, 2024 10:48

Delete unused function handler from create_page

4b7d09f

Use component instead of comp_name

f00f682

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

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

gmalinve Oct 17, 2024

DaveTwyman Oct 17, 2024

gmalinve Oct 17, 2024

DaveTwyman Oct 17, 2024 •

edited

Loading

gmalinve commented Oct 17, 2024

SMoraisAnsys left a comment

DaveTwyman commented Oct 18, 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

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

gmalinve Oct 17, 2024

Choose a reason for hiding this comment

DaveTwyman Oct 17, 2024

Choose a reason for hiding this comment

gmalinve Oct 17, 2024

Choose a reason for hiding this comment

DaveTwyman Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

gmalinve commented Oct 17, 2024

SMoraisAnsys left a comment

Choose a reason for hiding this comment

DaveTwyman commented Oct 18, 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

codecov bot commented Oct 11, 2024 •

edited

Loading

gmalinve commented Oct 14, 2024 •

edited

Loading

DaveTwyman Oct 17, 2024 •

edited

Loading