Add Li2O properties - #444
Conversation
Refs idaholab#419 AI-assisted: true AI-model: GPT-5.4 AI-version: gpt-5.4 Co-Authored-By: GPT-5.4 <codex@openai.com>
simopier
added
the
PR: AI assisted
|
Job Precheck, step Format Check Clang on 7a051d7 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:
|
Refs idaholab#419 AI-assisted: true AI-model: GPT-5.4 AI-version: gpt-5.4 Co-Authored-By: GPT-5.4 <codex@openai.com>
|
Job Build test summary, step Build test summary on b8f7f3a wanted to post the following: Test summaryCompared against 1cb89d5 in job civet.inl.gov/job/4042583. No change |
|
Job Documentation, step Sync to remote on bd08a98 wanted to post the following: View the site here This comment will be updated on new commits. |
Refs idaholab#419 AI-assisted: true AI-model: GPT-5.4 AI-version: gpt-5.4 Co-Authored-By: GPT-5.4 <codex@openai.com>
|
@GiudGiud @cticenhour this is ready for review. The only thing is the ad tests that I somehow can't figure out right now, so suggestions are welcome. This is providing the base structure for these types of classes and will be expanded later on. |
|
Job Precheck, step Python: black format on 337cb08 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: |
GiudGiud
left a comment
GiudGiud
left a comment
There was a problem hiding this comment.
will verify all 6 expressions later
code looks good
| | Enum | Expression used in TMAP8 | Property units | Validity range (K) | Reference | Notes | | ||
| | :- | :- | :- | :- | :- | :- | | ||
| | `Ohira1989Tritium` | $K_{s,T} = \exp(1290/T + 1.14)$ | atm$^{1/2}$ | 573.15-1000 K | [!cite](Ohira1989Li2O) | Reduced-species tritium dissolution in single-crystal Li2O | | ||
| | `Ohira1989Hydrogen` | $K_{s,T} = \exp(1271/T + 2.33)$ | atm$^{1/2}$ | 473.15-1000 K | [!cite](Ohira1989Li2O) | Reduced-species hydrogen dissolution in single-crystal Li2O | |
There was a problem hiding this comment.
what's the motivation behind having a hydrogen-specific correlation in this T-specific object?
There was a problem hiding this comment.
Not a very strong reason, but here were my thoughts: I found it easier to maintain, and I was also wondering if it was worth creating a brand new object for the different isotopes. I like having them all in one object/doc/figure to be able to compare and identify the isotopic effect.
Maybe we move it away into its own object later on when things are more mature (more models) and we want to differentiate things better?
| issued_warning = true; | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
not for now but a solutionInvalid option would be best
Co-authored-by: Guillaume Giudicelli <guillaume.giudicelli@gmail.com>
| { | ||
| using std::exp; | ||
|
|
||
| if (model == "Ohira1989") |
There was a problem hiding this comment.
this is a string comparison at every Qp. It's not ideal performance-wise. It'd be best to compare integers (e.g. the position in the enum)
even better would be to set A and Ea once in the material on initialSetup and no longer do any switches
We can keep performance items for later though
There was a problem hiding this comment.
Good point, I created an issue to optimize this once everything is in (it'll be easier to check for accuracy once we have the tests merged and we can see nothing is affected: #455
Also, I Iike setting A and Ea during initial setup a lot, but I dislike using integers instead of the enums since this makes the code less readable and therefore more error-prone. So that's what i suggested in the issue.
Co-authored-by: Guillaume Giudicelli <guillaume.giudicelli@gmail.com> Co-authored-by: Pierre-Clement Simon <pierreclement.simon@gmail.com>
|
Really great catch for the temperature ranges @GiudGiud! The paper actually shows some inconsistency in the temperature range. I now use the text from above Eq. (3) for the range, EXCEPT for the upper temperature for hydrogen since it is much smaller than what their Fig. (3) shows. I added an alert in the documentation too. I also made these corrections in the documentation and the python script. |
|
good on my side though I cannot resolve comments with that level of repo access fyi |
3 participants
Refs #419