idaholab / TMAP8

Tritium Migration Analysis Program, Version 8
https://mooseframework.inl.gov/TMAP8/
GNU Lesser General Public License v2.1
15 stars 18 forks source link

Adding val-2b case #59

Closed singhgp4321 closed 1 year ago

singhgp4321 commented 1 year ago

Ref: #12

singhgp4321 commented 1 year ago

@lindsayad @humrickhouse

moosebuild commented 1 year ago

Job Precheck on 75dd162 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/59/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 758bb22ae2a352cf3666f985150cfff15554986b

moosebuild commented 1 year ago

Job Documentation on b0dcfa6 wanted to post the following:

View the site here

This comment will be updated on new commits.

singhgp4321 commented 1 year ago

@lindsayad When you get a chance please add a paragraph on the convergence in the .md file. Do you recommend putting the validation cases under test/tests or in a separate directory which may be called "assessment".

lindsayad commented 1 year ago

I would say test/tests is good

singhgp4321 commented 1 year ago

@lindsayad Could you add a paragraph on guidelines for getting convergence for these kind of problems involving interface, as Paul was suggesting? The test for this case passes locally but fails the CIVET testing; it is csvdiffing right now. I am thinking of removing adaptive time stepping and fixing the time steps. Does that sound the right way to fix it?

lindsayad commented 1 year ago

It seems like there must be a solve failure, either in the run used to create the gold file, or on CIVET since the times are not the same length

singhgp4321 commented 1 year ago

There is no solve fail locally so it should be failing only on civet. I will try to create the civet environment on rod with help from @loganharbour and try to track it down.

simopier commented 1 year ago

Is there any documentation for SolubilityRatioMaterial that was added as part of this PR? I don't find any.

cticenhour commented 1 year ago

I was about to say that this should have failed CI if it wasn't there, but looks like we have WARNING turned on in the SQA config for missing markdown currently. 🤦🏻‍♂️

simopier commented 1 year ago

I was about to say that this should have failed CI if it wasn't there, but looks like we have WARNING turned on in the SQA config for missing markdown currently. 🤦🏻‍♂️

Can you update that setting? (I have no idea how to do it)

cticenhour commented 1 year ago

I will update it once everything is green (so as not to disrupt submodule updates). Beyond the documentation you mention, ReleasingNodalKernel, TrappingNodalKernel, EnclosureSinkNodalKernel, and EquilibriumBC are all missing class descriptions in the C++ source. This should also be fixed (ideally at the same time the documentation is added).

cticenhour commented 1 year ago

The issues noted above are being fixed up in #63.