Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EOSBuilder rewrite #311

Merged
merged 27 commits into from
Nov 29, 2023
Merged

EOSBuilder rewrite #311

merged 27 commits into from
Nov 29, 2023

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Oct 13, 2023

PR Summary

This has been a long time coming, as referenced in #28 and #133 . The EOSBuilder infrastructure inflexible, hard to extend, and slow. Moreover, as we move towards a model where each downstream code has its own variant (at least for the C++ codes), the hardcoded nature of EOSBuilder becomes an issue.

Here I refactor this machinery to allow a downstream code to be able to iteratively build up an equation of state from a base type and a nested chain of modifiers. For example:

using namespace singularity;
EOS eos = IdealGas(gm1, cv);
if (do_shift) {
  eos = EOSBuilder::Modify<ShiftedEOS>(eos, shift);
}
if (do_scale) {
  eos = EOSBuilder::Modify<ScaledEOS>(eos, scale);
}

This machinery is built off of some tricky templating nonsense. In particular, the Modify method recursively walks through all types in the variant to find the current underlying type of eos and modify it. This must be guarded by checks that the modifier applied to the eos object is still in the variant.

Playing these games also allows me to remove the fully EOS type list from the test_modifiers.cpp unit tests file and use a smaller type list, speeding up unit test compile times.

I was also able to remove eos_builder.cpp entirely. Just to speed up compile times, I added an eos.cpp file where some modifiers applied to EOS objects are declared, providing some code for the linker across translation units. This can be removed if desired, but doing so will require modifying the build system to support an entirely header-only build mode.

These kinds of games are easier in C++17 than in older standards, so it's quite likely the variatic code I wrote can be cleaned up once we jump standards. In particular if constexpr would be super useful here.

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 Yurlungur linked an issue Oct 13, 2023 that may be closed by this pull request
This was referenced Oct 13, 2023
test/test_eos_tabulated.cpp Show resolved Hide resolved
@@ -61,34 +117,14 @@ SCENARIO("EOS Builder and Modifiers", "[EOSBuilder][Modifiers][IdealGas]") {
REQUIRE(unmod.IsType<IdealGas>());
}
}
}
WHEN("We use the EOSBuilder") {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unified with the above test case, as EOSBuilder now no longer is a special code path.

@Yurlungur Yurlungur self-assigned this Oct 13, 2023
@Yurlungur Yurlungur added enhancement New feature or request interface Library interface work labels Oct 13, 2023
Copy link
Collaborator

@mauneyc-LANL mauneyc-LANL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to get a bit deeper into this. We may want to go about this differently; in particular, stacking type-lists and variant may be adding complexity we can avoid, and with what you have it may be just a little extra is enough to dump variant entirely.

singularity-eos/eos/eos_builder.hpp Outdated Show resolved Hide resolved
// TODO: Replace these with the new Modify method in EOSBuilder.
// NOTE: The new EOSBuilder machinery will likely be slower than these.
template <typename T>
EOS applyShiftAndScale(T &&eos, bool scaled, bool shifted, Real scale, Real shift) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dholladay00 if we end up keeping the rewrite, we should convert singularity_eos.cpp to use the new builder code at some point. But for now, I left the older applyXY* stuff, as it works without intervention. It may also be faster to compile on the full variant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New tests for some of the variatic functionality

@Yurlungur
Copy link
Collaborator Author

I finally had a chance to return to this.

@dholladay00 thanks for the suggestion of using CRTP to clean this up/make it more performant. The result of this is the following:

  1. I am still using the contains empty struct, to check whether or not a type is in a type list.
  2. However, all explicit tail recursion is gone. Since the base class knows the underlying type through CRTP, we can jump right to that spot in the variant and modify that object directly.

The public facing API is identical, plus an object-oriented API is available:

eos = eos.Modify<Modifier>(args);

now works, and I also provide

bool is_modifiable = eos.ModifiedInVariant<Mod>();

which checks that the current type can be modified and remain in the variant. In the variant, this code looks basically identical to all the other variant functions---just calls to visit.

Note that grossness couldn't be entirely removed. The problem is that mpark::visit requires that the visiting function always have the same return type. So in Modify, I need a visitor that always returns a variant with the same types. To make this happen, I needed some tag dispatch in the base class:

  // Modifies an EOS object by pulling out underlying type and modifying it
  // This one returns Mod<CRT>
  template <template <class> typename Mod, typename... Args>
  constexpr Mod<CRTP> Modify(Args &&... args) const {
    CRTP unmodified = *(static_cast<CRTP const *>(this));
    return Mod<CRTP>(std::move(unmodified), std::forward<Args>(args)...);
  }
  // These are overloads needed for the variant, as std::visit must be
  // able to return a variant of the same type every time. This lets
  // us do so, even though sometimes we don't modify the object.
  template <template <class> typename Mod, typename... Args>
  constexpr Mod<CRTP> ConditionallyModify(std::true_type, Args &&... args) const {
    return Modify<Mod>(std::forward<Args>(args)...);
  }
  template <template <class> typename Mod, typename... Args>
  constexpr CRTP ConditionallyModify(std::false_type, Args &&... args) const {
    CRTP unmodified = *(static_cast<CRTP const *>(this));
    return unmodified;
  }
  // Empty type list struct just used to capture "Ts" and "Args" simultaneously.
  template <template <class> typename Mod, typename... Ts, typename... Args>
  constexpr auto ConditionallyModify(const variadic_utils::type_list<Ts...> &tl,
                                     Args &&... args) const {
    constexpr bool do_mod = variadic_utils::contains_v<Mod<CRTP>, Ts...>();
    return ConditionallyModify<Mod>(variadic_utils::bool_constant<do_mod>(),
                                    std::forward<Args>(args)...);
  }

then the variant checks at runtime that you never hit ConditionallyModify(std::false_type,...):

  // EOS modifier object-oriented API
  template<template<class> typename Mod>
  constexpr bool ModifiedInVariant() const {
    return mpark::visit(
        [](const auto &eos) { return eos.template ModifiedInList<Mod, EOSs...>(); },
        eos_);
  }
  template<template<class> typename Mod, typename... Args>
  constexpr auto Modify(Args &&...args) const {
    PORTABLE_ALWAYS_REQUIRE(ModifiedInVariant<Mod>(), "Modifier must be in variant");
    return mpark::visit(
        [&](const auto &eos) {
          auto modified = eos.template ConditionallyModify<Mod>(
              variadic_utils::type_list<EOSs...>(), std::forward<Args>(args)...);
          return eos_variant<EOSs...>(modified);
        },
        eos_);
  }

Anyway I think this is in pretty good shape now.

@Yurlungur Yurlungur merged commit 6968515 into main Nov 29, 2023
4 checks passed
@Yurlungur Yurlungur deleted the jmm/build-rewrite branch November 29, 2023 17:47
@Yurlungur
Copy link
Collaborator Author

@dholladay00 says he's happy and tests pass. I'm pulling the trigger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request interface Library interface work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loop over types in the variant?
4 participants