-
Notifications
You must be signed in to change notification settings - Fork 34
Modularized functions for divertor_monoblock input file #337
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
base: devel
Are you sure you want to change the base?
Conversation
gonuke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @katielygre - this will definitely make things more readable. A couple of thoughts here for more clarity, from my perspective...
| ### Maximum mobile flux of 7.90e-13 at the top surface (1.0e-4 [m]) | ||
| ### 1.80e23/m^2-s = (5.0e23/m^2-s) *(1-0.999) = (7.90e-13)*(${tungsten_atomic_density})/(1.0e-4) at steady state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably makes sense to move this comment to the place where the max flux is defined
simopier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @katielygre!
The tests currently fail because you need to reference the issue in one of your commit message. Something like (Close #332). This is required for SQA reasons; it's not enough to reference the issue number in the description of the PR, you also need it in one of the commit message.
This is why you are getting the pre-check error:
##########################################################################
ERROR: Your patch does not contain a valid ticket reference! (i.e. #1234)
Merge branch 'devel' of https://github.com/katielygre/TMAP8 into test
Merge branch 'idaholab:devel' into devel
Added timing functions for plasma discharge
##########################################################################
gonuke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me - thanks @katielygre
|
@katielygre, your precheck failures are: See my comment above regarding Check out the guidance provided in the error message above to fix the whitespace issue. |
|
Job Documentation, step Sync to remote on fa58585 wanted to post the following: View the site here This comment will be updated on new commits. |
simopier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have additional comments for you, @katielygre, which you can find below.
I also noticed that the heavy test for the divertor monoblock model was failing. You were not predicting the same behavior as before your changes. So I looked more into it and found the issue, which I described here (#339) and fix here (#340). We'll need to wait for this change to be merged before your changes can be merged. As we discuss, your PR should really not change the behavior of the tests.
Also, if you have a bit of time, I would encourage you to do the same changes you do here in the input files divertor_monoblock_physics-single-var.i and divertor_monoblock_physics.i. I know I told you not to at first to keep things simple, but since you are handling this change well and will need to wait for the other PR to get in, you might as well do it all together here with my latest suggestions.
Regarding suggestions, to facilitate incorporating these comments and the following reviews, I suggest going to the Files changed tab and clicking "add to batch" to the suggestions to agree with, and then go to the top of the page and commit all the suggestions. This will automatically close the suggested changes, and will facilitate follow up reviews.
Let me know if you have any questions!
gonuke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you accidentally committed a merge conflict. Fortunately, it's only a single line and it's identical in both cases, so should be easy to fix.
| <<<<<<< HEAD | ||
| plasma_min_heat = 0.0 # W/m^2 # no flux while the pulse is off. | ||
| ======= | ||
| plasma_min_heat = 0.0 # W/m^2 # no flux while the pulse is off. | ||
| >>>>>>> d3e87cd (added t_in_cycles everywhere) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh-oh.... some git problems here....
|
Job Build test summary, step Build test summary on fa58585 wanted to post the following: Test summaryCompared against 99ed1f6 in job civet.inl.gov/job/3426101. No change |
|
@katielygre #340 is now merged, so you can now rebase, resolve conflict, and push this branch to update the PR. This should resolve the failing heavy tests. |
Co-authored-by: Pierre-Clement Simon <[email protected]>
…physics-single-var.i
|
You are missing a reference to an issue number in a commit message. You should have |
Resolves #332
Added a function for the general time history of a plasma discharge and for use in other functions.
Added variables to define the plasma discharge (ramp time, discharge time, maximum heat flux, maximum tritium flux, etc.)
Used variables to scale and shift the general time plasma function to match the original input file.