Skip to content

Full ranged ZrCoHx PCT modelling capabilities - #420

Open
Anthony-Bowers08 wants to merge 18 commits into
idaholab:develfrom
Anthony-Bowers08:ZrCoHx_FullRange_PCT_Modelling
Open

Full ranged ZrCoHx PCT modelling capabilities#420
Anthony-Bowers08 wants to merge 18 commits into
idaholab:develfrom
Anthony-Bowers08:ZrCoHx_FullRange_PCT_Modelling

Conversation

@Anthony-Bowers08

@Anthony-Bowers08 Anthony-Bowers08 commented May 30, 2026

Copy link
Copy Markdown
Contributor

Ref. #261

Reason

Design

Full range ZrCo PCT modelling capabilities

Impact

Full range ZrCo PCT modelling capabilities

@simopier simopier self-assigned this Jun 3, 2026

@simopier simopier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a partial review, not a full detailed one.

Please address these things first.

You are also missing an issue number in your commit history:

##########################################################################
ERROR: Your patch does not contain a valid ticket reference! (i.e. #1234)
Merge branch 'ZrCoHx_FullRange_PCT_Modelling' of https://github.com/Anthony-Bowers08/TMAP8 into test
Full ranged ZrCoHx PCT modelling capabilities Ref. 261
##########################################################################

Comment thread doc/content/source/interfacekernels/ADMatInterfaceReactionZrCoHxPCT.md Outdated
Comment thread doc/content/source/interfacekernels/ADMatInterfaceReactionZrCoHxPCT.md Outdated
Comment thread doc/content/source/interfacekernels/ADMatInterfaceReactionZrCoHxPCT.md Outdated
Comment thread doc/content/source/interfacekernels/ADMatInterfaceReactionZrCoHxPCT.md Outdated
Comment thread test/tests/ZrCo_hydrogen_system/ZrCoHx_PCT_Overall.i
Comment thread test/tests/ZrCo_hydrogen_system/ZrCoHx_PCT_Overall.i Outdated
Comment thread test/tests/ZrCo_hydrogen_system/ZrCoHx_PCT_Overall.i Outdated
Comment thread test/tests/ZrCo_hydrogen_system/ZrCoHx_PCT_Overall.i Outdated
Comment thread test/tests/ZrCo_hydrogen_system/ZrCoHx_PCT_Overall.i Outdated
@simopier

simopier commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

This PR is still failing due to:

##########################################################################
ERROR: Your patch does not contain a valid ticket reference! (i.e. #1234)
Merge branch 'ZrCoHx_FullRange_PCT_Modelling' of https://github.com/Anthony-Bowers08/TMAP8 into test
Apply suggestions from code review
Apply suggestion from @simopier
Full ranged ZrCoHx PCT modelling capabilities Ref. 261
##########################################################################

@moosebuild

moosebuild commented Jun 10, 2026

Copy link
Copy Markdown

Job Documentation, step Sync to remote on fcf3ec4 wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild

moosebuild commented Jun 10, 2026

Copy link
Copy Markdown

Job Coverage, step Generate coverage on fcf3ec4 wanted to post the following:

Coverage

9d9626 #420 fcf3ec
Total Total +/- New
Rate 90.71% 91.04% +0.33% 100.00%
Hits 1367 1402 +35 52
Misses 140 138 -2 0

Diff coverage report

Full coverage report

This comment will be updated on new commits.

@Anthony-Bowers08
Anthony-Bowers08 force-pushed the ZrCoHx_FullRange_PCT_Modelling branch from 6e56f25 to f898b6b Compare June 11, 2026 18:36
@Anthony-Bowers08
Anthony-Bowers08 force-pushed the ZrCoHx_FullRange_PCT_Modelling branch from f898b6b to 681717b Compare June 11, 2026 19:23
@lin-yang-ly lin-yang-ly reopened this Jun 11, 2026
@Anthony-Bowers08
Anthony-Bowers08 force-pushed the ZrCoHx_FullRange_PCT_Modelling branch from da77422 to f898b6b Compare June 11, 2026 20:11
@lin-yang-ly
lin-yang-ly force-pushed the ZrCoHx_FullRange_PCT_Modelling branch from f898b6b to f1d8f2f Compare June 11, 2026 20:48
@lin-yang-ly lin-yang-ly changed the title Full ranged ZrCoHx PCT modelling capabilities Ref. 261 Full ranged ZrCoHx PCT modelling capabilities Jun 11, 2026

@simopier simopier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting closer too! Let me know if you have any questions about these comments and suggestions.

Comment thread test/tests/ZrCo_hydrogen_system/ZrCoHx_PCT.i Outdated
Comment thread doc/content/source/interfacekernels/ADMatInterfaceReactionZrCoHxPCT.md Outdated
Comment thread doc/content/source/interfacekernels/ADMatInterfaceReactionZrCoHxPCT.md Outdated
Comment thread doc/content/source/interfacekernels/ADMatInterfaceReactionZrCoHxPCT.md Outdated
Comment thread test/tests/ZrCo_hydrogen_system/comparison_ZrCoHx_PCT.py Outdated
Comment thread test/tests/ZrCo_hydrogen_system/comparison_ZrCoHx_PCT.py Outdated
Comment thread test/tests/ZrCo_hydrogen_system/comparison_ZrCoHx_PCT.py Outdated
Comment thread test/tests/ZrCo_hydrogen_system/comparison_ZrCoHx_PCT.py Outdated
Comment thread test/tests/ZrCo_hydrogen_system/comparison_ZrCoHx_PCT.py Outdated
@moosebuild

Copy link
Copy Markdown

Job Precheck, step Python: black format on 7628199 wanted to post the following:

Python black formatting

Your 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:

curl -s https://mooseframework.inl.gov/tmap8/docs/PRs/420/black/black.patch | git apply -v

Alternatively, you can run the following at the top level of your repository:

black --config pyproject.toml --workers 1 .

@Anthony-Bowers08
Anthony-Bowers08 force-pushed the ZrCoHx_FullRange_PCT_Modelling branch from 7628199 to 82f2a6a Compare July 27, 2026 22:02

@simopier simopier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick partial review.

Comment thread doc/content/source/interfacekernels/ADMatInterfaceReactionZrCoHxPCT.md Outdated
Comment thread doc/content/source/interfacekernels/ADMatInterfaceReactionZrCoHxPCT.md Outdated
Comment thread doc/content/source/interfacekernels/ADMatInterfaceReactionZrCoHxPCT.md Outdated
Comment thread src/interfacekernels/ADMatInterfaceReactionZrCoHxPCT.C Outdated
@moosebuild

Copy link
Copy Markdown

Job Precheck, step Format Check Clang on 26c4624 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/tmap8/docs/PRs/420/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 121fdc4ef91b7bb6abb4581cf81e17a1a72b66e6

@simopier simopier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things look good overall but tests are failing.

@Anthony-Bowers08

Anthony-Bowers08 commented Aug 25, 2026 via email

Copy link
Copy Markdown
Contributor Author

@simopier simopier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small change

Comment thread doc/content/source/interfacekernels/ADMatInterfaceReactionZrCoHxPCT.md Outdated

@simopier simopier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You get one csvdiff:

DIFF test:ZrCo_hydrogen_system.ZrCoHx_PCT_Low_to_High_524K_csv FAILED (CSVDIFF)

In the documentation, MOOSEdocs has changed and now requires input files to be cross-referenced as [!file](/input_name.i). You will need to update these.

@simopier simopier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things look good overall, but you have added a bunch of csv files in the gold folder that are not tested. You should remove them.

@moosebuild

Copy link
Copy Markdown

Job Build test summary, step Build test summary on b5b0c36 wanted to post the following:

Test summary

Compared against 9d96263 in job civet.inl.gov/job/4115258.

Removed tests

Test Time (s) Memory (MB)
test:ZrCo_hydrogen_system.ZrCoHx_PCT_T604_5E4P_csv 3.67 125.25
test:ZrCo_hydrogen_system.ZrCoHx_PCT_T433_1E4P_csv 3.64 137.33
test:ZrCo_hydrogen_system.ZrCoHx_PCT_T573_1E4P_csv 3.63 123.25
test:ZrCo_hydrogen_system.ZrCoHx_PCT_T433_3E4P_csv 3.54 141.97
test:ZrCo_hydrogen_system.ZrCoHx_PCT_T604_6E3P_csv 3.52 168.39
test:ZrCo_hydrogen_system.ZrCoHx_PCT_T573_1E3P_csv 3.47 127.29
test:ZrCo_hydrogen_system.ZrCoHx_PCT_T433_1E2P_csv 3.07 166.42

Added tests

Test Time (s) Memory (MB)
test:ZrCo_hydrogen_system.ZrCoHx_PCT_T604_5E2P_csv 4.57 141.79
test:ZrCo_hydrogen_system.ZrCoHx_PCT_T524_3E4P_csv 4.32 126.43
test:ZrCo_hydrogen_system.ZrCoHx_PCT_Low_to_High_604K_csv 4.31 298.18
test:ZrCo_hydrogen_system.ZrCoHx_PCT_T423_1E4P_csv 4.16 142.44
test:ZrCo_hydrogen_system.ZrCoHx_PCT_T524_5E2P_csv 4.05 144.48
test:ZrCo_hydrogen_system.ZrCoHx_PCT_Low_to_High_524K_csv 3.92 204.13
test:ZrCo_hydrogen_system.ZrCoHx_PCT_Low_to_High_564K_csv 3.89 204.49
test:ZrCo_hydrogen_system.ZrCoHx_PCT_Low_to_High_423K_csv 3.63 170.58
test:ZrCo_hydrogen_system.ZrCoHx_PCT_Low_to_High_584K_csv 3.42 203.29
test:ZrCo_hydrogen_system.ZrCoHx_PCT_Low_to_High_624K_csv 2.83 295.90
test:ZrCo_hydrogen_system.ZrCoHx_PCT_Low_to_High_544K_csv 2.80 169.79

@Anthony-Bowers08
Anthony-Bowers08 force-pushed the ZrCoHx_FullRange_PCT_Modelling branch from b5b0c36 to fcf3ec4 Compare August 28, 2026 22:26
@simopier

Copy link
Copy Markdown
Collaborator

You get this failure in test summary and conda test linux (but not in tests). Maybe relax the rel_tol a bit:

DIFF ZrCo_hydrogen_system.ZrCoHx_PCT_Low_to_High_524K_csv FAILED (CSVDIFF)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants