Extended PCT capabilities to low pressure for YHx - #292
Extended PCT capabilities to low pressure for YHx#292Anthony-Bowers08 wants to merge 30 commits into
Conversation
|
Job Documentation, step Sync to remote on b916521 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.
Thank you for this @Anthony-Bowers08!
I have several high level requests/suggestions:
- Instead of adding a new material class, you should instead apply this in the existing
ADMatInterfaceReactionYHxPCTclass. There, you will see this if statement:
if (!_silence_warnings && ((neighbor_pressure < limit_pressure) || (neighbor_pressure > 1.e6)))
You should edit this if statement so that if the neighbor_pressure is above 1.e6 or lower than ~2.e2 (the lower pressure at which we have data if I remember properly), then an error is provided, if it is between 1.e6 and limit_pressure, then the current calculations for the high pressure regime is applied, and if it is between 2.e2 and limit_pressure, then it calculates the atomic fraction at the surface following the new equation you are proposing.
I think that should do it, and capture the plateau region. My concern is that we might see some oscillations around the plateau region, but we can test that and deploy a solution once we observe that.
-
I see that you have created an input file and submitted gold file, which is great. However, you need to also edit the
testfile related to this capability and add tests for all the configurations in the if statement above, with a particular focus on the new cases you are creating. The reason why thecoveragecurrently fails is because despite adding a new input file, no additional test is being run.
Note also that rather than create a brand new input file, you should instead use thecli_argsoption in thetestfile to utilize the existing file, but in the regime of interest to you. You will find example of that in thetestfile. -
You will also need to update the documentation page (format in
.md) to detail your new contribution to the existing capabilities. -
Your python script should be merged with the existing one for
ADMatInterfaceReactionYHxPCT. Do it all in there.
Let me know if you have any questions.
f0140fa to
05b1c7c
Compare
|
The prechecks are failing due to the white spaces: I thought you had applied the changes I had suggested to get rid of those. Did it not work? |
8dad514 to
6a315c7
Compare
|
@Anthony-Bowers08, you will need to fix the conflict with the python script. |
|
@simopier , think python script is an updated one from your prior .py yttrium script. I essentially added the low pressure plotting and made into a function based filed. I can revert to your prior .py file and just add the low pressure plotting there. |
Please do. Also, the conflicts means that the files have been updated in your branch and in a recent merge of TMAP8. So it's less about the nature of the change, and more about the fact that several changes from different origins are in conflict. We have recently deployed a formatting script for TMAP8, which affected many of the python scripts. That is probably the culprit. |
502a728 to
b70b205
Compare
simopier
left a comment
There was a problem hiding this comment.
You need an issue number in your commit message:
#########################################################################
ERROR: Your patch does not contain a valid ticket reference! (i.e. #1234)
Merge branch 'yttrium_hydride_lowpressure' of https://github.com/Anthony-Bowers08/TMAP8 into test
Modifications, reformatting, and cleaning YHX #Ref 262
##########################################################################
simopier
left a comment
There was a problem hiding this comment.
Thank you for this extension!
I added comments for you to incorporate.
Let me know if you have any questions.
| [YHx_PCT_Data] shows the data used in this interface kernel. The experimental data originates from [!cite](Lundin_1962). | ||
| The [YHx_PCT_fit_2D] shows high and low pressure fit with the high pressure fit from [!cite](Matthews2021SWIFT) and the low-pressure is a newly fitted curve. | ||
|
|
||
| !media comparison_YHx_PCT.py |
There was a problem hiding this comment.
Please use the same colors as for the following figure. You use the same type of color palette, but it would be easier for the reader if you used the same color for the same temperature in both figures.
| nl_rel_tol = 1e-4 | ||
| nl_abs_tol = 4e-9 |
There was a problem hiding this comment.
Lin is making changes to these cases, and you should be ready to use his options, which will be merged soon. See #382
There was a problem hiding this comment.
I am surprised that you have to change the tolerances so much. Did you try reducing them back down?
|
Job Precheck, step Python: black format on fdf51b4 wanted to post the following: Python black formattingYour code requires style changes. A patch was generated and copied here. You can directly apply the patch by running the following at the top level of your repository: Alternatively, you can run the following at the top level of your repository: |
simopier
left a comment
There was a problem hiding this comment.
This test diffs:
DIFF test:yttrium_hydrogen_system.YHx_PCT_T1273_P3e2_csv FAILED (CSVDIFF)
Things are really taking shape otherwise!
| if (neighbor_pressure > limit_pressure && abs(neighbor_pressure - limit_pressure) < tolerance) | ||
| { | ||
| // High pressure region, near limit | ||
| atomic_fraction = 0.5; |
There was a problem hiding this comment.
This comment still holds.
| // If near the start of the high pressure , jump to low pressure maximum atomic ratio | ||
| Ar_Max_LP_fit = 1.01e-6 * Utility::pow<2>(_neighbor_temperature[_qp]) - | ||
| 2.55e-3 * _neighbor_temperature[_qp] + 2.156; |
There was a problem hiding this comment.
Let's set up a meeting to chat about this. My calendar is up to date.
|
Job Build test summary, step Build test summary on 4d11a94 wanted to post the following: Test summaryCompared against 7d1a0b5 in job civet.inl.gov/job/3675371. Added tests
|
|
This PR needs to be properly rebased. |
Co-authored-by: Pierre-Clement Simon <pierreclement.simon@gmail.com>
Co-authored-by: Pierre-Clement Simon <pierreclement.simon@gmail.com>
Co-authored-by: Pierre-Clement Simon <pierreclement.simon@gmail.com>
Co-authored-by: Pierre-Clement Simon <pierreclement.simon@gmail.com>
Co-authored-by: Pierre-Clement Simon <pierreclement.simon@gmail.com>
Co-authored-by: Pierre-Clement Simon <pierreclement.simon@gmail.com>
Co-authored-by: Pierre-Clement Simon <pierreclement.simon@gmail.com>
aa036a6 to
576b451
Compare
simopier
left a comment
There was a problem hiding this comment.
I'm adding a few suggestions, but I'll need to see the full documentation to provide the full review. The precheck are currently filing due to clang format. The solve is provided in the comment above.
There was a problem hiding this comment.
Do not add the png files directly, create them with the python script.
There was a problem hiding this comment.
Will remove, thanks for pointing this out
| ax.set_ylabel("Partial Pressure (Pa)") | ||
| ax.grid(True, which="both", linestyle=":", alpha=0.6) | ||
|
|
||
| ncols_temp = min(len(temp_handles), 5) |
There was a problem hiding this comment.
please replace temp with temperature everywhere.
|
|
||
| # diffusivity from Majer et al., Journal of Alloys and Compounds 330-332 (2002) 438-442. | ||
| diffusivity_Do = '${units 1.e-8 m^2/s}' | ||
| diffusivity_Do = '${units 1 m^2/s}' |
There was a problem hiding this comment.
This is changed to reduce the time needed to reach steady-state converged solution.
| [function_BC_concentration_H_enclosure_1] | ||
| type = ParsedFunction | ||
| expression = 'exp(-${tau_constant_BC}/t)* ${initial_concentration_H_enclosure_1}' | ||
| expression = 'exp(-${dt_init}*2e-2/t)* ${initial_concentration_H_enclosure_1}' |
There was a problem hiding this comment.
Please put back tau_constant_BC. Why did you remove it?
| nl_rel_tol = 1e-4 | ||
| nl_abs_tol = 4e-9 |
There was a problem hiding this comment.
I am surprised that you have to change the tolerances so much. Did you try reducing them back down?
Co-authored-by: Pierre-Clement Simon <pierreclement.simon@gmail.com>
|
Job Precheck, step Format Check Clang on 51542d0 wanted to post the following: Your code requires style changes. A patch was auto generated and copied here
Alternatively, with your repository up to date and in the top level of your repository:
|
simopier
left a comment
There was a problem hiding this comment.
Some small comments, and also note that you have some failing tests:
[3.180s] [ 196MB] DIFF test:yttrium_hydrogen_system.YHx_PCT_T1273_P3e3_csv FAILED (CSVDIFF)
[15.75s] [ 234MB] DIFF test:yttrium_hydrogen_system.YHx_PCT_T1273_P3e2_csv FAILED (CSVDIFF)
[2.835s] [ 259MB] DIFF test:yttrium_hydrogen_system.YHx_PCT_T1473_P3e3_csv FAILED (CSVDIFF)
[9.379s] [ 300MB] DIFF test:yttrium_hydrogen_system.YHx_PCT_T1573_P5e3_csv FAILED (CSVDIFF)
Co-authored-by: Pierre-Clement Simon <pierreclement.simon@gmail.com>
Co-authored-by: Pierre-Clement Simon <pierreclement.simon@gmail.com>
(Ref. #261)