Ver 1kb

[Lee01Atom]

(Ref. #12)

[simopier]

@Lee01Atom, please update the comment at the top of the page to list the related issue (see what i did on your previous PR).

[simopier]

Here are some general comments after a rapid preliminary review:

• Please add a couple metrics to make this a verification case (we need comparison to an analytical solution. You could add a postprocessor to keep track of mass conservation (see tests in interfaceSorption), and another to measure the concentration jump at the interface and make sure that it is equal to the expected value from Henry's law (see tests in interfaceSorption).
• Please explain why you use both InterfaceDiffusion and InterfaceSorption. InterfaceSorption should be enough since it also accounts for diffusion through the interface. This might be over constraining your system. Note that InterfaceSorption also takes in the diffusivities (see Add val 2c to TMAP8 for Test Cell Experiment #197).
• You will need a tests file to run this case
• Your nl_abs_tol = 1e-6 and nl_rel_tol = 1e-6 are quite large (defaults are 1e-30 and 1e-8, respectively). Do you really need them to be this high? (see input files in interfaceSorption)
• I think your petsc options should be more like what is used in interfaceSorption tests

[moosebuild]

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

View the site here

This comment will be updated on new commits.

[moosebuild]

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

Coverage

Coverage did not change

Full coverage report

This comment will be updated on new commits.

[simopier]

You currently get the following error:

##########################################################################
ERROR: The following files contain trailing whitespace after applying your patch:
	test/tests/ver-1kb/ver-1kb.i

Run the "delete_trailing_whitespace.sh" script in your $MOOSE_DIR/scripts directory.
##########################################################################

Check both these links out to automatically solve these issues in VSCode:

1. https://mooseframework.inl.gov/help/development/VSCode.html
2. https://stackoverflow.com/questions/30884131/remove-trailing-spaces-automatically-or-with-a-shortcut

[simopier]

Thanks for the update @Lee01Atom!

1. you are missing the alias for comparison_ver-1kb.py in the doc/content/verification_and_validation/figures folder.
2. The documentation currently does not show any verification, in the sense that it does not verify mass conservation or the fact that the solubility law is properly computed. You need to check these metrics in a quantitative way to make it a valuable verification case. (see the last figure in the documentation in Clean up and Update val-2b #202 as an example).

[simopier]

You're getting there. The main remaining gaps are (1) the documentation, which meeds more more content to detail the case and explain the results and quantitatively show that TMAP8 accurately models this case in a way (see specific comments), and (2) the solubility value.

I looked at the input file and the case a little bit, and I think I understand why you are not getting the expected results.

Henry's law states C2 = KP1, and the TMAP7 case explains that the pressures of the two enclosures are expected to equilibrate at the same value, which is also shown in their figure with the results. For that reason, I believe that they defined the solubility Kin a way that ensured that at equilibrium: P1=P2, or C2 = C1. Since C2 = KP1 = K C R T, I think they picked K = 1/R/T. The reason you are not getting the same result as them with the same K value is because you are not using the same units. If you set up K = 1 / R / T, you will be able to reproduce their results.

However, I don't think this is a a very interested result since the concentration jump at the interface is effectively 0. So here's what I think would make this case more valuable:

Create two different sub-cases within this case, one with (a) K = 1 / R / T, and one with (b) K = 10 / R / T or any other value to show that TMAP8 can reproduce the TMAP7 case without any jump (i.e., a) and model a concentration jump at the interface (i.e., b). For both case, plot the pressure evolution over time in both enclosure, show that mass is conserved, and show that the concentration jump is properly simulated (0 for sub-case a, and KRT for sub-case b).

You can create subcase (b) simply by creating a new test in the tests file and update the solubility value with the cli_agrs argument (see case ver-1jb as an example on how to do that).

Please let me know if you have any questions, or if anything is unclear.

[simopier]

@Lee01Atom, here are some additional comments that should help you fix the failure in the test environment and improve this case.

Let me know if you have any questions. You're on the right track!

[simopier]

The heavy test fail is unrelated to this PR.

[simopier]

Thanks @Lee01Atom! Here are a few comments/suggestions for improvements.

Let me know if you have any questions.

[simopier]

You will need to update the gold files since we changed the simulation time.

[simopier]

Some new comments.

Before pushing new commits, make sure to run the tests and build the documentation locally to fix any easy-to-catch bugs/failures/issues. That will accelerate the development process and limit the number of iterations here.

Another thing: I think you should make the simulation time even shorter since most of the interesting changes happen on the very left side of the figures. You should also update the time units from hours to seconds, at that point.

Let me know if you have any questions.

[simopier]

Some minor comments.

[simopier]

I'm wondering something: Could you refine the node size (and increase the number of nodes) and see the impact it has on mass conservation? I think we could improve it.

As you do that, make sure to update the documentation accordingly.

[Lee01Atom]

I'm wondering something: Could you refine the node size (and increase the number of nodes) and see the impact it has on mass conservation? I think we could improve it.

As you do that, make sure to update the documentation accordingly.

When I increase the number of nodes (e.g., from 100 to 1,000), the percentage of mass variation indeed decreases by a factor of 10.
However, the test for ver-1kb_exodus_concentration_jump fails because two elements have midpoints so close that Exodiff cannot distinguish them.

[simopier]

It sees like you update part of the documentation, but not the input file.

[simopier]

I'm wondering something: Could you refine the node size (and increase the number of nodes) and see the impact it has on mass conservation? I think we could improve it.

As you do that, make sure to update the documentation accordingly.

When I increase the number of nodes (e.g., from 100 to 1,000), the percentage of mass variation indeed decreases by a factor of 10.
However, the test for ver-1kb_exodus_concentration_jump fails because two elements have midpoints so close that Exodiff cannot distinguish them.

There are ways to avoid this failure with Exodiff not recognizing that two points are different because they are too close. For example, we can update the length unit of the case so that the number representing the distance is above the exodiff threshold (which is an absolute value without units directly associated with it). or we can use a .exodiff file to modify the that tolerance and reduce it.
In this case, I think it is reasonable to use 100 nodes in the mesh, and acknowledge in the documentation that the mass variations can be further minimized by using a finer mesh.

[simopier]

Looks great, thank you so much for this contribution, @Lee01Atom!

I think the remaining cases should be easy with your current experience, but feel free to reach out with any questions.