-
Notifications
You must be signed in to change notification settings - Fork 34
[WIP] Deuterium retention in neutron-irradiated single-crystal tungsten #325
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
4b39d0e to
c937c4f
Compare
135be67 to
4c88923
Compare
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 @cticenhour! I'm excited for this case to be added!
I left comments and suggestions even though this is still tagged as [WIP] because I was too curious to ignore it. It looks like the main thing missing is the documentation, correct?
| # Setup mask using a random sampling method to reduce density of scatter plot points in experimental | ||
| # data (there is a lot of "noise"). | ||
| # These preserve 10% of the data points. (adjusted within the 'size' parameter). | ||
| mask = np.random.choice(len(experiment_flux_673), size=int(len(experiment_flux_673) * 0.1), replace=False) |
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 would like to see, outside of this PR, the figures both with and without this mask to see why it's needed.
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.
Sent you a message just now on Teams - interested in hearing what you think! Left some context on "why" there as well.
| # tmap_flux_for_rmspe = numerical_solution_on_experiment_input(experiment_time, simulation_time_TMAP7, simulation_flux_left_TMAP7/2 + flux_environment) | ||
| # RMSE = np.sqrt(np.mean((tmap_flux_for_rmspe-experiment_flux)**2) ) | ||
| # RMSPE = RMSE*100/np.mean(experiment_flux) | ||
| # ax.text(6000.0,0.85e18, 'RMSPE = %.2f '%RMSPE+'%',fontweight='bold') |
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.
Why is this commented out? Maybe it's just because this is still in WIP stage, but calculating the RMSPE between TMAP8's predictions and the experimental data is a good thing to add.
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 was commented out because it added complexity while I was working through adjusting the rest of the plot. It is planned to be re-added before this is merged.
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.
Could you add units in the column names?
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.
Added in adad02e.
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.
Could you add units in the column names?
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.
Added in adad02e.
| [] | ||
|
|
||
| [Postprocessors] | ||
| [flux_surface_left] |
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.
Specify units as a comment.
| @@ -0,0 +1,321 @@ | |||
| # General parameters | |||
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.
Please add a comment at the top of the input files to detail what validation case it reproduce.
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.
Added in 58ea5cd.
| [BCs] | ||
| [left] | ||
| type = ADDirichletBC | ||
| variable = concentration | ||
| boundary = left | ||
| value = 0 | ||
| [] | ||
| [right] | ||
| type = ADDirichletBC | ||
| variable = concentration | ||
| boundary = right | ||
| value = 0 | ||
| [] | ||
| [] |
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 is a reasonable assumption, but I want to highlight that this assumption was shown to significantly affect the results compared to accounting for finite recombination kinetics at surfaces in val-2f (https://mooseframework.inl.gov/TMAP8/verification_and_validation/val-2f.html). I'm not saying the same thing should be done here, but maybe it could be mentionned in the documentation of this validation case with a link to val-2f.
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 understand plotting the temperature history this way, but I think there is also value in plotting the tmeperature history based on the temperature from the csv outputs of the TMAP8 simulations. The advantage of doing so is that it will be updated along with the TMAP8 input files (if it changes or is adapted for a different history), and would clearly highlight any potential issues with the implementation of the temperature history in the input file. It also prevents from having the temperature history implemented in two different places (this scipt and the input file). Let me know what you think.
Confirmed that temperature and diffusivity profiles over time look reasonable, and there is a reasonably high trapped population, but zero mobile concentration. Convergence fails over only a couple of timesteps.
…timestepper, and lower trap_per_free parameter
…ture ploting script
…concentration, and other parameter adjustments for clarity
…ts with the source publication
…settings in input file, that did not impact results.
Use random sampling to reduce the density of the scatter plot of the experimental data - similar to what Masa's doing in the paper. Cannot tell what approach he is using the reduce the plot density. As of this change, 10% of the data will be sampled and shown on the plot using the random sampler.
176cba9 to
79d875a
Compare
Co-authored-by: Pierre-Clement Simon <pierreclement.simon@gmail.com>
…ectly from the simulation. Add temperature function to postprocessor output for use in scripts. Start output of simulation data from timestep 0 to get the full temperature series.
ffd05b1 to
a9d8db5
Compare
Masa's Paper: https://www.sciencedirect.com/science/article/pii/S0920379618303880