-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add 1ka verification case #178
Conversation
Thank you for your contribution, @Lee01Atom! The tests currently fail because your commit messages do not reference an existing issue:
to solve it, follow the instructions from the "contributing to TMAP8" page :
Right now, you should be able to run:
|
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.
@Lee01Atom, this is very quick first take looking at the main missing pieces, not a thorough review yet:
- you should also add a documentation page (
.md
) that describes the case. you can find examples inprojects/TMAP8/doc/content/verification_and_validation/
- You should also add a
test
file that will run the test. You can find examples in the other V&V cases. - as explained above, you need your commit message to list the issue number.
These requirements are (in part) what ensure that TMAP8 remains Nuclear Quality Assurance, level 1 compliant. It takes a bit of time to get used to this way of doing code development, but thorough documentation and rigorous testing are key to high software quality assurance. It helps build trust in TMAP8.
Please let me know if you have any questions.
(Ref. idaholab#12)
Job Documentation, step Sync to remote on f3dc432 wanted to post the following: View the site here This comment will be updated on new commits. |
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.
Preliminary review with some comments and suggestions.
@@ -0,0 +1,27 @@ | |||
import numpy as np |
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.
Add in-code comments
import numpy as np | ||
import pandas as pd | ||
import matplotlib.pyplot as plt | ||
|
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.
Look at other .py files for up to date practices:
- Automatic figure generation
- calculate RSMPE
input = val-1ka.i | ||
csvdiff = val-1ka_out.csv | ||
requirement = 'The system shall be able to model the tritium volumetric source in one enclosure' | ||
[] |
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.
Add test case to run the python script.
Example:
[ver-1a_comparison]
type = RunCommand
command = 'python3 comparison_ver-1a.py'
requirement = 'The system shall be able to generate comparison plots between the analytical solution and simulated solution of verification case 1a, modeling species diffusion through a structure, originating from a depleting source enclosure.'
required_python_packages = 'matplotlib numpy pandas os'
[]
test/tests/ver-1ka/ver-1ka.md
Outdated
!media figures/ver-1ka_comparison_time.png | ||
style=width:50%;margin-bottom:2% | ||
id=ver-1ka_comparison_time | ||
caption=Comparison of $T_2$ partial pressure in an enclosure with no loss pathways as function of time calculated through TMAP8 and analytically |
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.
We now use a different approach to ensure the figures are automatically generated by the documentation. Here is an example:
!media comparison_ver-1g.py
image_name=ver-1g_comparison_equal_conc.png
style=width:50%;margin-bottom:2%;margin-left:auto;margin-right:auto
id=ver-1g_comparison_equal_conc
caption=Comparison of concentration of AB as a function of time calculated through TMAP8 and analytically for the case when A and B have equal concentrations.
Apply preliminary suggestions Co-authored-by: Pierre-Clement Simon <[email protected]>
Cleaning this up was a challenge, so another PR was opened to replace this one. See #183. |
(Ref. #12)
EDIT: @Lee01Atom, you had the issue number in the comments, so it did not show. I just fixed it.