Vinet EOS implemented

[aematts]

PR Summary

The coding is done in eos_vinet.hpp and it is verified to get the correct numbers out.
Unit test cases are in test_eos_vinet.cpp, testing all Vinet related functions and some more. A little more work is needed here to finalize the unit tests.
No documentation is provided yet.

PR Checklist

• [x ] Adds a test for any bugs fixed. Adds tests for new features.
• [x ] Format your changes by using the make format command after configuring with cmake.
• [x ] Document any new features, update documentation for changes made.
• Make sure the copyright notice on any files you modified is up to date.
• After creating a pull request, note it in the CHANGELOG.md file

[jhp-lanl]

@aematts thanks for putting this together! I may not get to reviewing this until Thursday, but I plan to give it a thorough review then

[aematts]

I spent most of yesterday trying to figure out how to check that the code throws correctly when entering faulty model parameters (like negative density). This is for the unit testing. I found that Catch2 has functions like CHECK_THROWS_WITH( expression, string or string matcher ), which is exactly what I need. However, I cannot get it to work. I use EOS_ERROR which is defined (in base/eos_error.hpp) either as a throw or as an assert(false) and I use it in the constructor (in the routine CheckVinet()). In test_eos_vinet.cpp the last set of tests, in (Scenario: Vinet EOS SetUp) I get the tests to print out all error messages but I have not been able to check on this with Catch2. Can someone with superior C++ knowledge take a look at this?

[aematts]

You might need some documentation to understand my code. This is the writeup I will use for the docs eventually.
OnlyVinet.pdf

[jhp-lanl]

I spent most of yesterday trying to figure out how to check that the code throws correctly when entering faulty model parameters (like negative density). This is for the unit testing. I found that Catch2 has functions like CHECK_THROWS_WITH( expression, string or string matcher ), which is exactly what I need. However, I cannot get it to work. I use EOS_ERROR which is defined (in base/eos_error.hpp) either as a throw or as an assert(false) and I use it in the constructor (in the routine CheckVinet()). In test_eos_vinet.cpp the last set of tests, in (Scenario: Vinet EOS SetUp) I get the tests to print out all error messages but I have not been able to check on this with Catch2. Can someone with superior C++ knowledge take a look at this?

I'm guessing that the tests don't compile with SINGULARITY_ENABLE_EXCEPTIONS by default. @Yurlungur any ideas here?

[jhp-lanl]

I would recommend splitting the SNLGruneisen EOS into another MR so that we can review the two pieces of code separately (and because they shouldn't interact). Otherwise it looks like you still need to flesh out some of the tests, right?

Overall this looks great! The two main changes I would like to see are

1. I think for loops will be better than while loops for two reasons: (a) the scope of the counter variable is kept local to the loop so there's no subtlety about what the initial value of the counter is, and (b) I think for loops are in general more clear since the operation of the loop is on one line.
2. I'd like to discuss the best way to input the power series parameters. As it stands now, all 39 parameters must be specified, but since we just require a pointer, I feel it could be dangerous if a user accidentally passes an array of the wrong size or something. Otherwise, if the power series tends to require the leading terms but not the later terms, maybe it's better to have the user specify how many terms they are providing.

[jhp-lanl]

When you're ready, @jonahm-LANL has a shell script at utils/scripts/format.sh to automatically modify the code to comply with the formatting requirements. It requires the clang compiler, which I haven't been able to get working on snow, but is available on darwin.

[Yurlungur]

I spent most of yesterday trying to figure out how to check that the code throws correctly when entering faulty model parameters (like negative density). This is for the unit testing. I found that Catch2 has functions like CHECK_THROWS_WITH( expression, string or string matcher ), which is exactly what I need. However, I cannot get it to work. I use EOS_ERROR which is defined (in base/eos_error.hpp) either as a throw or as an assert(false) and I use it in the constructor (in the routine CheckVinet()). In test_eos_vinet.cpp the last set of tests, in (Scenario: Vinet EOS SetUp) I get the tests to print out all error messages but I have not been able to check on this with Catch2. Can someone with superior C++ knowledge take a look at this?

I'm guessing that the tests don't compile with SINGULARITY_ENABLE_EXCEPTIONS by default. @Yurlungur any ideas here?

Correct. Actually there's no cmake flag to turn it on. We can add that in this MR. @aematts I will try to add something to your branch and push tomorrow.

[Yurlungur]

When you're ready, @jonahm-LANL has a shell script at utils/scripts/format.sh to automatically modify the code to comply with the formatting requirements. It requires the clang compiler, which I haven't been able to get working on snow, but is available on darwin.

I believe I've used it on snow. I can try to get you a recipe.

[Yurlungur]

Thanks for putting this together @aematts ! I'm sorry it took me so long to review. Overall this is a great start and I'm happy to see you got your hands dirty with singularity-es so quickly. Hopefully it hasn't been too painful of an experience for you.

I see many C-isms in this code which we probably want to change to C++-isms, as well as some non-GPU aware code, but we can get to that in time.

I also have some questions about performance/readability/robustness below.

[Yurlungur]

I feel my comments in my above review were not as constructive as they could have been. I'd like to rectify that. I have submitted a new PR #209 outlining how interact on a pull request, which I will take to heart. I'm sorry for my terseness before.

Below I'd like to add a few words on some of my review comments:

On passing a raw pointer into the constructor

@mauneyc-LANL and @aematts are right about this and I'm wrong. I still think the optimal solution here is to pass in a std::vector<Real>& instead of a pointer. But there's nothing inherently wrong about passing in a pointer.

On global variables and on class fields

When possible it's best to avoid global variables and scope, as they can make the code harder to read because a reader needs to jump around a file to understand what's going on. Instead it's best to pass variables into class methods when possible. This is what motivated my comment about _expconsts

On "magic numbers"

"Magic numbers" is a common phrase in software meaning an integer such as "4" or "5" just lying around the code. To a casual reader of the program it can be difficult to infer the purpose or meaning of such a number. It's better to name the number by setting it to a constant variable. (Note these can be global and this is an exception to the rule of no global variables I mentioned above.)

On brackets around loop bodies

I understand that this one is controversial and also feels like a nitpick. I'm willing to back off on it. But here's my reasoning. Suppose you have a loop of the form

for (int i = start; i < end; i++)
  DoSomething(i);

and you decide you want modify that loop body. Perhaps you want to add a print statement printing the iteration for debugging purposes. It's very easy to accidentally write something like

for (int i = start; i < end; i++)
  printf("i = %d\n", i);
  DoSomething(i);

but now only the printf is inside the loop body, not DoSomething. That's why it's better to enclose the loop body in brackets, as it avoids little errors like these.

On while loops

The while loops you had written look like they are well posed and work perfectly well. The fear is that if someone comes into the code later and modifies them, the condition for their termination can be accidentally made invalid so the loop never terminates. That's why, in my opinion, it's best to try to cast loops as for loops as possible so it's harder for someone who comes into the code later to accidentally break it.

[aematts]

I still have not found a way to check on the throws. But this is ready for new reviews. I will not do anything until after the holidays so you have some time.

[aematts]

OK. I think this is done now.

[Yurlungur]

I'm pushing to re-git to trigger tests on Darwin. If those pass and @jhp-lanl and @dholladay00 are happy I will click the button.

[Yurlungur]

@jhp-lanl can you please go through and check if your requested changes have been made?

[Yurlungur]

re-git caught some extra errors on GPU builds in the tests. I resolved them. I also commented out the unused variables for the reference state just to remove compiler warnings. I like you added them @aematts and I left the code, just commented it out to make the compiler happy.

[Yurlungur]

Looks like the Kokkos tests are still failing for Vinet. I will try to take a look ASAP so we can get this in.

[aematts]

Looks like the Kokkos tests are still failing for Vinet. I will try to take a look ASAP so we can get this in.

I have just done this as they were in the MG. I really do not have a clue how to use that stuff. So I cannot help at all in checking this out. Sorry.

[Yurlungur]

Looks like the Kokkos tests are still failing for Vinet. I will try to take a look ASAP so we can get this in.

I have just done this as they were in the MG. I really do not have a clue how to use that stuff. So I cannot help at all in checking this out. Sorry.

Yeah not your fault and not a big deal. Just requires a little digging.

[dholladay00]

Is this being checked against the gitlab-ci @Yurlungur? I don't see this MR there

[dholladay00]

Is this being checked against the gitlab-ci @Yurlungur? I don't see this MR there

nvm I found it @Yurlungur

[jhp-lanl]

As long as @dholladay00 's suggestions about changing to portableFor loops is incorporated, I approve. My requested changes are minor.

[jhp-lanl]

CI on gitlab is failing for GPUs with a Kokkos::View ERROR: attempt to access inaccessible memory space error. It looks like it's coming from the

Scenario: Vinet EOS rho sie
      Given: Parameters for a Vinet EOS
      Given: Densities and energies
       When: A S(rho, e) lookup is performed

test. Is this just the portableFor issue?

[Yurlungur]

CI on gitlab is failing for GPUs with a Kokkos::View ERROR: attempt to access inaccessible memory space error. It looks like it's coming from the

Scenario: Vinet EOS rho sie
      Given: Parameters for a Vinet EOS
      Given: Densities and energies
       When: A S(rho, e) lookup is performed

test. Is this just the portableFor issue?

LIkely. I'm trying to test with this fixed.

[Yurlungur]

Tests on re-git now pass. Any last-minute objections before I merge?

[jhp-lanl]

Tests on re-git now pass. Any last-minute objections before I merge?

G2G

[Yurlungur]

Merged. Thanks @aematts for all your work on this and @jhp-lanl @dholladay00 @mauneyc-LANL for all your helpful engagement.

Looking forward to kinetic phase transitions next. :)