Add the ability to pull an unmodified EOS out of a modified one #254

Yurlungur · 2023-04-11T23:19:53Z

PR Summary

Sometimes you may wish to have access to the unmodified model... for example, because you wish to access model-specific features (i.e., the mass fractions in the stellar collapse tables), because you want to drop out of code units and use cgs, or because you wish to swap one modifier for another at runtime.

This PR lets you do those things. I provide three functions, shared across all models, that let you do this:

bool Model::IsModified() const returns true if the EOS has a modifier applied to it and false otherwise.
auto UnmodifyOnce() if the EOS is modified, pull off the most recent modifier. Returns either the variant or an appropriate type.
auto GetUnmodifiedObject() returns the underlying EOS with all modifiers removed. Type again is either the variant or the appropriate type.

This feature is needed by the NGP codes but I believe it will be useful broadly for any downstream code that uses C++. (Obviously it's of no help to the fortran codes.)

PR Checklist

Adds a test for any bugs fixed. Adds tests for new features.
Format your changes by using the make format command after configuring with cmake.
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
If preparing for a new release, update the version in cmake.

Yurlungur · 2023-04-11T23:25:50Z

singularity-eos/eos/eos_base.hpp

+  // Tooling for modifiers
+  PORTABLE_FORCEINLINE_FUNCTION
+  bool IsModified() const { return false; }
+  PORTABLE_FORCEINLINE_FUNCTION
+  auto UnmodifyOnce() { return *static_cast<CRTP *>(this); }
+  PORTABLE_FORCEINLINE_FUNCTION
+  auto GetUnmodifiedObject() { return *static_cast<CRTP *>(this); }


These provide default behavior for the "unmodified" EOS models. The modifiers must implement these differently and shadow them, (although there's an obvious template).

Yurlungur · 2023-04-11T23:26:29Z

singularity-eos/eos/modifiers/eos_unitsystem.hpp

+  PORTABLE_FORCEINLINE_FUNCTION
+  bool IsModified() const { return true; }
+  PORTABLE_FORCEINLINE_FUNCTION
+  T UnmodifyOnce() { return t_; }
+  PORTABLE_FORCEINLINE_FUNCTION
+  auto GetUnmodifiedObject() { return t_.GetUnmodifiedObject(); }


Unfortunately, these are copy-pasted into each modifier definition. It's maybe possible to add them all with a macro or some other trick, although I don't know how worthwhile that is.

Two tier CRTP? There would be the standard base class and then a modifier base class that inherits from the standard base class and provides these functions. I played around with it a bit a while back and it is doable.
https://stackoverflow.com/a/18174442/3491638

Hmm... that could work. If we add more modifier-specific stuff, let's consider it.

For consideration: mixins fit this pattern well, tho (from experience) I can say using mixins and crtp can be give tongue-twisty code. https://www.fluentcpp.com/2017/12/12/mixin-classes-yang-crtp/

If @jhp-lanl wants to make an PR, I'd be interested to see how tongue-twisty it is. May be worth it, though the current duplication overhead is small and not likely to be modified often.

@mauneyc-LANL I was looking at mixins as well a little bit ago but I thought that they weren't as elegant with our Variant class. We define a closed set of functions that are available for all types, whereas I feel a mixin is about expanding the functionality. If we were using class inheritance for our type erasure, maybe a mixin would be the preferable approach, but with the Variant, I lean towards CRTP since the end user doesn't have to change anything.

However, I wonder if a mixin could enable functionality like this?

EOS my_modified_eos = Shifted<Scaled<IdealGas(...)>>>; EOS once_unmodified = UnmodifyOnce(my_modified_eos); EOS unmodified_eos = UnmodifyAll(my_modified_eos);

Yurlungur · 2023-04-11T23:27:07Z

test/test_eos_unit.cpp

@@ -232,7 +232,7 @@ SCENARIO("EOS Variant Type", "[Variant][EOS]") {
  std::cout << demangle(typeid(EOS).name()) << std::endl;
 }

-SCENARIO("EOS Builder and Modifiers", "[EOSBuilder],[Modifiers][IdealGas]") {
+SCENARIO("EOS Builder and Modifiers", "[EOSBuilder][Modifiers][IdealGas]") {


This was a harmless bug. The comma never should have been there. We need it not there, in fact, to correctly parse this string for categories with catch.

dholladay00 · 2023-04-11T23:45:07Z

@jhp-lanl I can't remember exactly, but I want to make sure that this won't break any plans for vector interface modifiers. There is something about multiple parallel_fors but I am forgetting why. I don't think we modify any underlying data such that an unmodify would require (un)modifying internal EOS state (like tables), but I'd like you to think that through and double check me on that.

jhp-lanl · 2023-04-12T00:03:33Z

@jhp-lanl I can't remember exactly, but I want to make sure that this won't break any plans for vector interface modifiers. There is something about multiple parallel_fors but I am forgetting why.

I think the underlying issue was with modifiers being applied to EOSPAC. Currently the modifiers inherit from the base class, their default vector behavior is to call their own scalar functions which modify scalar EOS calls. In the EOSPAC case, we need the vector modifier to call the vector EOS call with the appropriate modifiers applied to the inputs/outputs.

I remember we had discussed whether this required another set of parallel for loops or whether there was a better way that maybe could use serial for loops to apply the modifiers and then use the parallel for to apply the modifier. This might be less performant for EOS other than EOSPAC though, so we would want to do some profiling.

I don't think we modify any underlying data such that an unmodify would require (un)modifying internal EOS state (like tables), but I'd like you to think that through and double check me on that.

If we did apply the modifiers to the data rather than the inputs/outputs, you'd be correct that this might be a non-trivial operation, but right now we don't do that. I'd advocate for trying out a simple for loop to modify inputs/outputs and assess the performance impact from such an approach before we go down the road of modifying internal class states.

EDIT - There are also probably EOSPAC options we could use to let EOSPAC do the modifying, but I'd like to test just simple for loops against a "pure" EOSPAC implementation to see how much of a detriment is incurred. Getting the EOSPAC options to work in a general modifier framework would be a challenge.

jhp-lanl

I don't like the copy/paste but it's not enough for me not to approve this pending the fix to the doc issue.

Yurlungur · 2023-04-12T00:22:46Z

I'm actually not sure what's going on with the docs. The docs built, but the deploy stage failed because the hash is wrong. Very strange.

Yurlungur · 2023-04-12T00:40:47Z

Must have been a momentary thing. Docs build now.

Yurlungur · 2023-04-12T01:35:33Z

Gotten approval from @jhp-lanl and(verbally) from @dholladay00 so I'm merging it. Please feel free to add feedback after the fact though. We're not set in stone here.

mauneyc-LANL · 2023-04-12T16:35:11Z

singularity-eos/eos/eos_variant.hpp

+  PORTABLE_FORCEINLINE_FUNCTION
+  bool IsModified() const {
+    return mpark::visit([](const auto &eos) { return eos.IsModified(); }, eos_);
+  }
+  PORTABLE_FORCEINLINE_FUNCTION
+  Variant UnmodifyOnce() {
+    return mpark::visit(
+        [](auto &eos) { return eos_variant<EOSs...>(eos.UnmodifyOnce()); }, eos_);
+  }
+  PORTABLE_FORCEINLINE_FUNCTION
+  Variant GetUnmodifiedObject() {
+    return mpark::visit(
+        [](auto &eos) { return eos_variant<EOSs...>(eos.GetUnmodifiedObject()); }, eos_);
+  }


There's a discussion to have here about how data is being copied. Shortly: Rather than construct new objects, it would be better to for the visit pattern to filter out modifiers (see the example at https://en.cppreference.com/w/cpp/utility/variant/visit for "type-matching" visitor).

I don't have any wunderkind one-liner to make this happen, and unfortunately all my immediate ideas involve some moderately sized refactors (the C++14 restrict is also very constraining).

Anyway, as is it would be worth double-checking that copying(constructing) these objects isn't going to have any ill-considered side-effects. For example, do the Databox's get duplicated? This could lead to issues with memory ownership conflicts. (it may not, but just pointing to a possibility)

All EOS's are assumed to have reference semantics, including databoxes, so copying will not cause correctness problems, though it may cause performance bottlenecks.

I was not aware that one could do a type-matching visit. That's cool. There may be other uses for that.

format

ffa5c0a

Yurlungur added the enhancement New feature or request label Apr 11, 2023

Yurlungur requested review from dholladay00, brryan, lroberts36, AstroBarker, mauneyc-LANL and jhp-lanl April 11, 2023 23:19

Yurlungur self-assigned this Apr 11, 2023

changelog

c82d466

Yurlungur commented Apr 11, 2023

View reviewed changes

jhp-lanl approved these changes Apr 12, 2023

View reviewed changes

typo

fbcee05

Yurlungur merged commit b4c9ce7 into main Apr 12, 2023

Yurlungur deleted the jmm/unmodify branch April 12, 2023 01:35

mauneyc-LANL reviewed Apr 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to pull an unmodified EOS out of a modified one #254

Add the ability to pull an unmodified EOS out of a modified one #254

Yurlungur commented Apr 11, 2023 •

edited

Loading

Yurlungur Apr 11, 2023

Yurlungur Apr 11, 2023

jhp-lanl Apr 11, 2023

Yurlungur Apr 12, 2023

mauneyc-LANL Apr 12, 2023

dholladay00 Apr 12, 2023

jhp-lanl Apr 12, 2023

Yurlungur Apr 11, 2023

dholladay00 commented Apr 11, 2023

jhp-lanl commented Apr 12, 2023 •

edited

Loading

jhp-lanl left a comment •

edited

Loading

Yurlungur commented Apr 12, 2023

Yurlungur commented Apr 12, 2023

Yurlungur commented Apr 12, 2023

mauneyc-LANL Apr 12, 2023

Yurlungur Apr 13, 2023

Add the ability to pull an unmodified EOS out of a modified one #254

Add the ability to pull an unmodified EOS out of a modified one #254

Conversation

Yurlungur commented Apr 11, 2023 • edited Loading

PR Summary

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholladay00 commented Apr 11, 2023

jhp-lanl commented Apr 12, 2023 • edited Loading

jhp-lanl left a comment • edited Loading

Choose a reason for hiding this comment

Yurlungur commented Apr 12, 2023

Yurlungur commented Apr 12, 2023

Yurlungur commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yurlungur commented Apr 11, 2023 •

edited

Loading

jhp-lanl commented Apr 12, 2023 •

edited

Loading

jhp-lanl left a comment •

edited

Loading