Refactoring plot_proc.py for use in fusrr#2979
Merged
timothy-nunn merged 11 commits intomainfrom Jan 12, 2024
Merged
Conversation
clmould
force-pushed
the
cmould/updating-plot-proc
branch
from
e954d9c to
a0aa88f
Compare
clmould
force-pushed
the
cmould/updating-plot-proc
branch
from
d655865 to
e7eb63e
Compare
clmould
changed the title
clmould
force-pushed
the
cmould/updating-plot-proc
branch
2 times, most recently
from
9ca7e21 to
50dec2a
Compare
clmould
force-pushed
the
cmould/updating-plot-proc
branch
from
72a788e to
7ae6e6a
Compare
clmould
force-pushed
the
cmould/updating-plot-proc
branch
from
c20e0e6 to
bf173b2
Compare
clmould
force-pushed
the
cmould/updating-plot-proc
branch
from
658219c to
9a5b4f6
Compare
Contributor
|
From my end, this LGTM! |
timothy-nunn
requested changes
Dec 20, 2023
Collaborator
timothy-nunn
left a comment
timothy-nunn
left a comment
There was a problem hiding this comment.
It looks like a lot of comments, but it basically boils down to:
- Clearing up exactly the types that are contained in the dataclasses and ensuring they have a well-defined API.
- Discussing if consolidation of some of the dataclasses into one unified
ArbitraryGeometrywould make sense (this is not me saying we should do that, it is something for us to discuss) - Clarifying the names of some functions that are called 'plot...' but dont seem to do any plotting.
- Clarifying the docstrings (types and descriptions) on functions that have lots of returns. This is a complicated sub-package and it will get confusing what does and returns what without more documentation, and I want using this to be as easy as possible for any user of PROCESS.
- Variable naming to, again, aide in clarity and understanding
- Some other minor comments about fudge factors. I see they were in old plot_proc so it might a case of you asking one of the modelers.
Other than that, the sub-package is laid out well, docstring format is all correct, and plot_proc is looking a lot less busy!
- add arbitrary geometry class - remove i_single_null < 0 option in plasma_geometry.py - add better descriptions for RectangleGeometry objects
timothy-nunn
approved these changes
Jan 12, 2024
Collaborator
timothy-nunn
left a comment
timothy-nunn
left a comment
There was a problem hiding this comment.
I am happy with the code. Could you maybe comment a before and after from plotproc of the large tokamak just to show nothing has changed?
Collaborator
Author
|
plot_proc output for large tokamak before: after: |
Collaborator
|
Thanks Clair, all looks good to me! Thanks for your work on this! |
chris-ashe
pushed a commit
that referenced
this pull request
Apr 22, 2024
* refactoring plot_proc adding dataclass * removed todo comments * update variable description shldith in large_tokamak_IN.DAT * Added docstrings to refactored work * refactor firstwall plotting * restructure blanket and first wall geometries * improve blanket and firstwall plotting * rename vars * improve vac vessel and shield plotting * initial changes based on PR review comments * make changes based on review comments - add arbitrary geometry class - remove i_single_null < 0 option in plasma_geometry.py - add better descriptions for RectangleGeometry objects
chris-ashe
pushed a commit
that referenced
this pull request
Apr 22, 2024
* refactoring plot_proc adding dataclass * removed todo comments * update variable description shldith in large_tokamak_IN.DAT * Added docstrings to refactored work * refactor firstwall plotting * restructure blanket and first wall geometries * improve blanket and firstwall plotting * rename vars * improve vac vessel and shield plotting * initial changes based on PR review comments * make changes based on review comments - add arbitrary geometry class - remove i_single_null < 0 option in plasma_geometry.py - add better descriptions for RectangleGeometry objects
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refactoring
plot_proc.pyfor use in fusrr. Parts ofplot_procwere copied and pasted intofusrrso refactoring these parts into separate files to be accessed by bothplot_procandfusrrwithout anything being copied and pasted between the two. The parts being refactored here are functions for the plasma, tf coils, pf coils, cryostat, blanket, shield and vacuum vessel.Also refactored function for the first wall and added first wall plotting into
fusrr.Checklist
I confirm that I have completed the following checks: