lanl / singularity-eos

Performance portable equations of state and mixed cell closures
https://lanl.github.io/singularity-eos/
BSD 3-Clause "New" or "Revised" License
25 stars 8 forks source link

Add introspection to `get_sg_eos` interface #382

Closed jhp-lanl closed 2 months ago

jhp-lanl commented 2 months ago

PR Summary

When the library is built with debug flags, this adds introspection functions to the init and final functors in the get_sg_eos interface.

These functions check to make sure the values being passed are either zero or normal. I'm using slightly more complicated logic than just is_finite so that I can detect underflow values as well. For some quantities, I'm also including checks to make sure the returned value is strictly positive or non-negative where appropriate.

PR Checklist

If preparing for a new release, in addition please check the following:

jhp-lanl commented 2 months ago

@Yurlungur @dholladay00 is going to be a bit unavailable for the next few days so I might need your review here.

Also please check me on whether you agree with what I'm requiring to be non-negative or strictly positive.

jhp-lanl commented 2 months ago

FYI - I did some reading on and vs && and it seems like the consensus is that because && has been used traditionally, it is preferred over and. Coming from Fortran and python, I find and more readable, but I'm happy to change them to the more commonly-used C++ symbols.

Yurlungur commented 2 months ago

FYI - I did some reading on and vs && and it seems like the consensus is that because && has been used traditionally, it is preferred over and. Coming from Fortran and python, I find and more readable, but I'm happy to change them to the more commonly-used C++ symbols.

I strongly prefer &&. I wasn't even aware and was valid C++.

jhp-lanl commented 2 months ago

FYI - I did some reading on and vs && and it seems like the consensus is that because && has been used traditionally, it is preferred over and. Coming from Fortran and python, I find and more readable, but I'm happy to change them to the more commonly-used C++ symbols.

I strongly prefer &&. I wasn't even aware and was valid C++.

Apparently it's a holdover from the ancient times when some keyboards had limited symbols believe it or not. Again, I just liked it because coming from a non-C++ background it seems more readable. I'll change it

jhp-lanl commented 2 months ago

@jonahm-LANL or @mauneyc-LANL any ideas on what I'm doing wrong here?

/home/runner/work/singularity-eos/singularity-eos/singularity-eos/eos/singularity_eos.f90:410: undefined reference to `get_sg_eos'

I added the mass_frac_cutoff argument to the get_sg_eos() interface function both on the C++ side and the fortran side, and I'm adding an optional argument to the fortran interface. I can't for the life of me see where my mistake is and why the linker wouldn't be able to see get_sg_eos(). Any help is appreciated!

jhp-lanl commented 2 months ago

@jonahm-LANL or @mauneyc-LANL any ideas on what I'm doing wrong here?

/home/runner/work/singularity-eos/singularity-eos/singularity-eos/eos/singularity_eos.f90:410: undefined reference to `get_sg_eos'

I added the mass_frac_cutoff argument to the get_sg_eos() interface function both on the C++ side and the fortran side, and I'm adding an optional argument to the fortran interface. I can't for the life of me see where my mistake is and why the linker wouldn't be able to see get_sg_eos(). Any help is appreciated!

@dholladay00 with the fix. I forgot that the get_sg_eos() prototype exists in the singularity_eos.hpp header even though the function itself is defined the get_sg_eos.cpp file.

jhp-lanl commented 2 months ago

@mauneyc-LANL and @Yurlungur let me know if the most recent changes have addressed your comments. I think the new error_utils.hpp file needs a fresh review

jhp-lanl commented 2 months ago

I'm having trouble with one machine on our gitlab CI (network cloning issues... possibly proxy problems). Can one of you run the CI for me there? I think this is good to go otherwise.

Yurlungur commented 2 months ago

I'm having trouble with one machine on our gitlab CI (network cloning issues... possibly proxy problems). Can one of you run the CI for me there? I think this is good to go otherwise.

Looks like it's passing on gitlab. Going to pull the trigger.