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

Fixed Docstring #3970 #3985

Merged
merged 27 commits into from
Dec 22, 2023
Merged

Fixed Docstring #3970 #3985

merged 27 commits into from
Dec 22, 2023

Conversation

DaveTwyman
Copy link
Collaborator

@DaveTwyman DaveTwyman commented Dec 14, 2023

Close #3970

@DaveTwyman DaveTwyman self-assigned this Dec 14, 2023
@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 bug Something isn't working label Dec 14, 2023
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0e2e1c2) 81.41% compared to head (9b29f46) 81.41%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3985   +/-   ##
=======================================
  Coverage   81.41%   81.41%           
=======================================
  Files         182      182           
  Lines       63306    63306           
=======================================
  Hits        51542    51542           
  Misses      11764    11764           

pyaedt/maxwell.py Outdated Show resolved Hide resolved
Co-authored-by: Maxime Rey <[email protected]>
@gmalinve
Copy link
Collaborator

gmalinve commented Dec 15, 2023

@MaxJPRey PR opened for training purposes :)
I'm pretty sure someone from the team will take care of this and will fix all the issues.
Remember to fix also the SetupTemplateMaxwell.rst mentioned in the linked issue.
@IreneWoyna @DaveTwyman @tizianrot @anur7

Copy link
Collaborator

@gmalinve gmalinve left a comment

Choose a reason for hiding this comment

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

@IreneWoyna @tizianrot @DaveTwyman could someone pick this up please?
We need to finish editing the docstring + the SetupMaxwellTemplate.rst file in the doc.
If needed please ping me :)

@DaveTwyman
Copy link
Collaborator Author

@IreneWoyna @tizianrot @DaveTwyman could someone pick this up please? We need to finish editing the docstring + the SetupMaxwellTemplate.rst file in the doc. If needed please ping me :)

Yes, I can finish this off, one of the checks is failing though, I'll message you

Copy link
Member

@Samuelopez-ansys Samuelopez-ansys left a comment

Choose a reason for hiding this comment

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

Please commit these changes

pyaedt/maxwell.py Outdated Show resolved Hide resolved
pyaedt/maxwell.py Outdated Show resolved Hide resolved
pyaedt/maxwell.py Outdated Show resolved Hide resolved
@gmalinve
Copy link
Collaborator

@DaveTwyman checks fail because we didn't finish to work on the docstring and some indentations are wrong

DaveTwyman and others added 4 commits December 20, 2023 09:56
Extra Line

Co-authored-by: Samuel Lopez <[email protected]>
Unneeded Indentation

Co-authored-by: Samuel Lopez <[email protected]>
Co-authored-by: Samuel Lopez <[email protected]>
pyaedt/maxwell.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@DaveTwyman DaveTwyman left a comment

Choose a reason for hiding this comment

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

Looks ok, I'll add the missing Maxwell solver types as well

DaveTwyman and others added 3 commits December 20, 2023 11:05
MIssing Maxwell solver types added
Spread Maxwell solver names over two lines to obey line length limit
Copy link
Collaborator Author

@DaveTwyman DaveTwyman 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

pyaedt/maxwell.py Outdated Show resolved Hide resolved
Hi, There was no deep thought behind the dash next to the colon. 
I've removed it, sounds like text will now be consistent with other parts of docs

Co-authored-by: SMoraisAnsys <[email protected]>
@gmalinve
Copy link
Collaborator

gmalinve commented Dec 21, 2023

@DaveTwyman just a suggestion and based on the training sessions we had, it's better to put a meaningful comment for each commit you make. So far you have pushed all the commits with the same message "update maxwell.py" .
image

It's not a problem but it's better to have a short message related to the change in your commit so it's easier for the review process :)

pyaedt/maxwell.py Outdated Show resolved Hide resolved
pyaedt/maxwell.py Outdated Show resolved Hide resolved
DaveTwyman and others added 8 commits December 21, 2023 20:38
The word 'Depending' was proposed instead of 'Based'

Co-authored-by: Kathy Pippert <[email protected]>
Solvers no longer grouped in terms of Magnetic and Electric solvers but now alphabetized and with double backticks and quotation marks applied.

Co-authored-by: Kathy Pippert <[email protected]>

.. pprint:: pyaedt.modules.SetupTemplates.MaxwellTransient
.. pprint:: pyaedt.modules.SetupTemplates.Magnetostatic
.. pprint:: pyaedt.modules.SetupTemplates.Electrostatic
.. pprint:: pyaedt.modules.SetupTemplates.EddyCurrent
.. pprint:: pyaedt.modules.SetupTemplates.ElectricTransient

.. pprint:: pyaedt.modules.SetupTemplates.ACConduction
Copy link
Collaborator

Choose a reason for hiding this comment

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

@DaveTwyman please have a look at the doc build checks that fails. It says that in SetupTemplates there are no templates named "ACConduction" or "DCConduction".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gmalinve , Ok, they can be added in the future. Maybe at the same time as adjusting all the other solver templates that reference HFSS. I've removed them for now

Copy link
Collaborator

Choose a reason for hiding this comment

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

@DaveTwyman did you remove them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gmalinve , I deleted the two lines in Pycharm and then hit commit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@DaveTwyman as you can see these lines are still here, let me try to do it on my side.

@SMoraisAnsys
Copy link
Collaborator

DaveTwyman

@DaveTwyman if you ever want to try to rewrite your branch history (e.g. changing commit names, squashing commits, do not hesitate to contact me)

Copy link
Collaborator Author

@DaveTwyman DaveTwyman left a comment

Choose a reason for hiding this comment

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

Removed reference to AC and DC Conduction templates


.. pprint:: pyaedt.modules.SetupTemplates.MaxwellTransient
.. pprint:: pyaedt.modules.SetupTemplates.Magnetostatic
.. pprint:: pyaedt.modules.SetupTemplates.Electrostatic
.. pprint:: pyaedt.modules.SetupTemplates.EddyCurrent
.. pprint:: pyaedt.modules.SetupTemplates.ElectricTransient

.. pprint:: pyaedt.modules.SetupTemplates.ACConduction
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gmalinve , Ok, they can be added in the future. Maybe at the same time as adjusting all the other solver templates that reference HFSS. I've removed them for now

Copy link
Collaborator Author

@DaveTwyman DaveTwyman left a comment

Choose a reason for hiding this comment

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

Removing this makes sense, I now see this is the maxwell.py file and not the rst file

@DaveTwyman DaveTwyman merged commit f0af285 into main Dec 22, 2023
12 checks passed
@DaveTwyman DaveTwyman deleted the fix/doc3970 branch December 22, 2023 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Maxwell create_setup docstring, currently referring to HFSS
6 participants