Clean up Verification case 1a

[simopier]

Commit 1:

• Clean up input file (re-organize, add comments, regroup data and equations at the top, update names, specify units)
• Edit number of significant digits used in constants to correspond to TMAP4/7 cases
• Re-gold affected test
• Remove seemingly unused gold file

Commit 2:

• Update python script to plot results

Commit 3:

• Use values for PhysicalConstants consistent with PhysicalConstants.h
• Rename test outputs to differentiate from regular output

(Ref. #75)

[simopier]

@cticenhour @RemDelaporteMathurin @ChaitanyaBhave-e @singhgp4321

This is a draft PR with what I have to contribute right now. This was me exploring the behavior of the current ver-1a test and cleaning things up. There is more to come before this should be merged:

• The documentation will be improved to provide more details about the TMAP8 analytical solution, the values used in this case, and a comment on the difference between the TMAP4 andTMAP7 cases (which we need to fully understand).
• Concerning the differences between TMAP4 and TMAP7, it seems like they are looking at different things. We need the TMAP8 case to compare well to both, and document both comparisons.
• I have not yet plotted/re-derived the TMAP4 analytical solution myself, this will be an important next step, and will ensure that the expression proposed in the TMAP4 V&V manual is correct (or not, as Rémi suggested)
• I was not able to reproduce the plot shown in the documentation. Even before my changes, the match was not perfect. The results have drifted and we need to figure out why - It might be due to using more accurate values for physical constants in kernels in TMAP8. Once we have a better understanding of the analytical solution from the TMAP4 case and can plot it with accurate physical constants, it'll probably be easier to figure this out.
• The plot in the documentation needs to go to 140 s, which we will be able to do after re-deriving/plotting the TMAP4 analytical solution.

This is all open to discussions/suggestions/comments/questions.

[moosebuild]

Job Documentation on 4516954 wanted to post the following:

View the site here

This comment will be updated on new commits.

[RemDelaporteMathurin]

• I was not able to reproduce the plot shown in the documentation. Even before my changes, the match was not perfect. The results have drifted and we need to figure out why - It might be due to using more accurate values for physical constants in kernels in TMAP8. Once we have a better understanding of the analytical solution from the TMAP4 case and can plot it with accurate physical constants, it'll probably be easier to figure this out.

I had also noticed that. It cannot be due to 'more accurate physical constants' since theoretically, the same constants are used in both the computed solution and the analytical solution.

What I've noticed when comparing TMAP8 and FESTIM to the analytical solution was that, when increasing the mesh density, there was a better agreement with the analytical solution

Original mesh:

10x more cells:

• I was not able to reproduce the plot shown in the documentation.

Imo this is an issue: the fact that the figures shown in the documentation cannot be reproduced by the scripts. This is something I already noticed in https://github.com/idaholab/TMAP8/issues/64 where the code had to be tweaked in order to get closer to what's shown in the documentation. This may be a bit misleading for users

I don't know how that could be possible with this CI architecture but it'd be nice if there was a way to generate these figures when compiling the docs instead of having "hard-coded" png images.

For example, in the HTM documentation, the images generated by the examples are not hard-coded and are generated on-the-fly by the code blocks in the docs. https://h-transport-materials.readthedocs.io/en/latest/user/examples.html

Here is the RST file for this page.

It would be nice to have something similar here.

• Concerning the differences between TMAP4 and TMAP7, it seems like they are looking at different things. We need the TMAP8 case to compare well to both, and document both comparisons.

They definitely are different metrics (even though both called $FR$). One (TMAP4) seems to be looking at cumulative surface flux when the other (TMAP7) is looking at surface flux and evolution of the relative pressure. With FESTIM we were able to compare against all these metrics with good agreement: the scenario is the same, just the metrics change.

[simopier]

I had also noticed that. It cannot be due to 'more accurate physical constants' since theoretically, the same constants are used in both the computed solution and the analytical solution.

Since I did not plot the analytical myself for now (but used the table provided in TMAP4), the number of significant digit is actually different in the analytical solution and TMAP8 in my case.

But I just refined the mesh and it does indeed match the analytical solution now. ✅

@RemDelaporteMathurin , I completely agree with your point about having the figure hard-coded vs. updated. We'll get on that. In addition, I agree that having an image that cannot be easily reproduced by the input file is problematic. Sometimes it's not reasonable because it would make the test too slow to run, but the documentation should at least clearly specify what needs to be done to reproduce this figure. In that case, I'll make the input file have a finer mesh and the test file use a coarser mesh to still have it run quickly.

@RemDelaporteMathurin, thanks again for your contribution!

[simopier]

The input file now outputs the flux and the release measured on both sides of the slab, and I was able to get, as expected, the TMAP7 data. WARNING: It still needs to be thoroughly compared to analytical solutions.

This is how it currently looks, with a finer mesh as Rémi suggested earlier:

@cticenhour, I tried to add a heavy test with a fine mesh and small time step, which are required to accurately reproduce the analytical solution (at least for TMAP4, I have not tried TMAP7 yet). It runs in ~9.5 seconds so it probably shouldn't be a regular test. I tried making it heavy but it still runs with the other tests, which makes me think that we don't have heavy tests set up. Could you help out with this?

[cticenhour]

A few things:

I don't know how that could be possible with this CI architecture but it'd be nice if there was a way to generate these figures when compiling the docs instead of having "hard-coded" png images.

@RemDelaporteMathurin we can generate images on-the-fly in our documentation system from a Python script with no issue. This also serves as a regular test of the scripts in these folders from now on, which I have been pushing for lately. I'll work with PC on getting this added to this PR.

I tried making it heavy but it still runs with the other tests, which makes me think that we don't have heavy tests set up. Could you help out with this?

@simopier If I remember correctly we run tests on the Linux target with the --all flag, which would always run heavy tests as well as non-heavy. I can work on getting this split out.

[RemDelaporteMathurin]

I'll work with PC on getting this added to this PR

That's great!

Surely not related to this PR, but generally a proper way to test these cases would be to compute the error between the analytical solution and the computed solution and ensuring it is below some criterion. I think that's more rigorous than checking against gold standard, hard-coded files.

If we want to push it, since it's FEM, one should check the convergence rates of the error and ensures that, as the mesh size increases, the error decreases at the expected rate (power law).

Another way to test these codes is using the Method of Manufactured Solutions since it provides a very robust and reliable way to test codes against complex cases (multi-dimensional, multi-materials, etc). Maybe something to keep in mind in the future.

[simopier]

Tests fail only because the new heavy test (which currently runs with other tests) times out - as expected - during debug tests. So once Casey sets up the heavy tests separately, we should be all good.

[chaibhave]

@simopier we should also update the input file so the length unit conversion is done using the MOOSE input file syntax which allows unit change. This automatically documents what units we are changing to/from and keeps things more concise and clear.

[simopier]

@simopier we should also update the input file so the length unit conversion is done using the MOOSE input file syntax which allows unit change. This automatically documents what units we are change to/from and keeps things more concise and clear.

Sounds great! Let's get this one in, and then I'll let you update it in a following PR.

[simopier]

Here are the current results:

The release fraction on the inner layer of the slab (corresponding to TMAP7's verification case) is not yet a perfect fit. I've been trying to look at the effect of mesh size, time step size, numerical options for analytical derivation, execution time, etc. But to no avail. I'm missing something still.

EDIT: THIS HAS NOW BEEN FIXED.

[simopier]

@RemDelaporteMathurin, I was trying to give you some credit with the last commit since I used your script but I must have done something wrong, we'll get it fixed. -Fixed, thank you @cticenhour for the help!

On a different note, I found a little typo in your script that explains the results you were getting: You used

summation = (2 * L * sec - np.exp(-(roots**2) * D * t / l**2)) / (L * (L + 1) + roots**2)

instead of

summation = (2 * L * sec * np.exp(-(roots**2) * D * t / l**2)) / (L * (L + 1) + roots**2)

(notice the - instead of the * before the exponential term.

(with this fix and other little updates, the analytical solution is matched by TMAP8 predictions - see above graph)

[RemDelaporteMathurin]

On a different note, I found a little typo in your script that explains the results you were getting

This is actually hilarious! Thank you so much for looking into it. This is proof that you can re-read a script 50 times and still miss the mistake! This means that the equation I derived (from TMAP7 analytical solution) and the one given by TMAP4 are equivalent. Must admit my maths background is too weak to know why... 😄

I reran the code with the fix and confirm everything overlaps!

[simopier]

This is actually hilarious! Thank you so much for looking into it. This is proof that you can re-read a script 50 times and still miss the mistake!

Tell me about it, I read through your script several times without seeing it. It's only when I rewrote it that I saw it! That's why collaborative/open-source projects like this one are very effective. Just like you did when you created this issue, we can double check each other and help each other out!

This means that the equation I derived (from TMAP7 analytical solution) and the one given by TMAP4 are equivalent. Must admit my maths background is too weak to know why...

Yeah, I plotted them and they indeed match. At that point, I'm not going to dig into why they are equivalent. My next steps are understanding why I'm not perfectly matching for TMAP7 release fraction, and improve the documentation.

[RemDelaporteMathurin]

why I'm not perfectly matching for TMAP7 release fraction

I think (not sure of anything anymore) that I was using the same parameters in FESTIM as with TMAP8, and had pretty good agreement for all these metrics:

EDIT: forgot the FESTIM legend on the last one but it's the solid blue line

Maybe something in the post-processing?

[simopier]

Maybe something in the post-processing?

@ChaitanyaBhave-e helped me figure out what was wrong (thanks!), I was using the variable u (concentration in slab) instead of looking at v (enclosure pressure) compared to the initial pressure. It now matches!

[simopier]

This is now complete and is ready for review (@cticenhour, @ChaitanyaBhave-e). The only thing to update is one cross-reference to the PhysicalConstants documentation, which I am adding to TMAP8 in a different PR: #92.

[simopier]

@cticenhour @ChaitanyaBhave-e ready for review!

[cticenhour]

@simopier FYI - the earliest I can come back to this for a final look will likely be tomorrow afternoon (~1:30PM MST).

[cticenhour]

Just sending a note to say that I haven't forgotten about this. Last week was crazy writing a conference paper, and this week I am starting it off sick, so I will block some time to finish this off when I can. 🤒

[cticenhour]

The complexity of my suggested changes for adding Python script testing and MooseDocs generation of figures required I push a couple commits for @simopier's review. I still need to go through a few more items on my checklist, but I wanted to get this in testing.