-
Notifications
You must be signed in to change notification settings - Fork 19
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 validation case val2a #189
Conversation
@simopier @chaibhave This is the draft PR for validation case 2a. Just let me know if you have any suggestions or comments for the model I built. That would be helpful for me to build the correct model. Thank you! |
Job Documentation, step Sync to remote on 4755cf1 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.
Thank you @lin-yang-ly.
A couple preliminary comments.
Plus, don't forget to list val-2a in the V&V list in index.md
.
@simopier I just updated the PR with your comments. Just let me know if you have more suggestions! |
8f74f38
to
8d9a306
Compare
@simopier @chaibhave I just upload the updated model. The result is finally close to the experimental result. The two figures below are the permeation flux on the right side based on data from TMAP4 and TMAP7, respectively. The result based on TMAP4 is close to experiment, except for the small bumps around 9000s and 15000s. However, the result based on TMAP7 is far away from experiment. I think it is because lots of data from TMAP7 are wrong. Based on current result, I think
Just let me know if you have any suggestions about that. Thank you all a lot for helping me figure this out! |
Thank you for the suggestion! I used the recombination flux instead of diffusion flux to compare with experimental results in the updated figures. Because only recombination flux ( |
Morning @simopier , I just removed the data and model from TMAP7, now it is ready for review! |
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.
Great stuff! I left quite a few comments, but you're getting very close. Let's discuss this.
Thank you for your hard work, @lin-yang-ly!
33b856a
to
006cfb1
Compare
(Ref. idaholab#188)
Co-authored-by: Pierre-Clement Simon <[email protected]>
Co-authored-by: Pierre-Clement Simon <[email protected]>
006cfb1
to
52c1ffe
Compare
@simopier I finally solved the issue about different number on time column of csvdiff test. I used the larger tolerance and a constant time step in the test case. Besides, I have several questions for the PR: Do we need to add exodus test for heavy case? The reason why I ask is I found the exodus file is around 500 MB in heavy case with finer mesh and time step. I try the adaptivity mesh method from the input file you sent to me, and I found the adaptivity mesh is not as good as the previous one. Because the previous mesh is the hand-crafted refined mesh. However, the adaptivity mesh may not find the reasonable mesh as soon as possible. This makes the running time longer than previous. The simulation may need at least two days to calculate the final results. Do you think we should keep current adaptivity mesh or turn back to the hand-crafted refined mesh? Except for these questions, the PR is ready for review. |
use constant time step update tolerance update tolerance 2 high tolerance with low time step loose tolerance lower tolerance mum to nm for better converge update output parameters alter time step update time step update time step maybe very high tolerance A test for better performance roll back Apply suggestions from code review Co-authored-by: Pierre-Clement Simon <[email protected]>
bd761cc
to
5e0dce1
Compare
5e0dce1
to
165bc2b
Compare
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 seems like you override a previous commit that contained my suggestions. I unresolved these few conversations. Please make sure not to do it in the future.
I added some more suggestions and requests, mostly for the documentation.
The last thing we need to figure out if how to reduce the tolerance (and potentially increase the time step) of this case. I'll work on it and let you know if I figure something out.
Co-authored-by: Pierre-Clement Simon <[email protected]>
Morning @simopier, I just updated the PR with your comments. Just let me know if you have more suggestions! |
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.
A couple more comments. Getting close!
Co-authored-by: Pierre-Clement Simon <[email protected]>
Thank you, PC! Just updated with your comments. Ready for review again! |
- use AD to improve convergence - tighten tolerances - increase max time step to reduce computational time - remove heavy tests since the test now runs in less than 3 seconds - update documentation - regold
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.
@simopier asked me to leave a few small comments on top of his last commit, and I found a few more items that should be addressed. This looks great overall!
Co-authored-by: Casey Icenhour <[email protected]>
Thank you for the comments! @cticenhour |
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.
Looks good to me! Thank you for this contribution, @lin-yang-ly!!
@cticenhour, it's ready for your final review. |
(Ref. #188)
(Ref. #12)