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

Add a new class, ExpressIn, to help with type erasure of quantities in Mathematica output #2544

Merged
merged 15 commits into from
Apr 26, 2020

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented Apr 25, 2020

  1. All Mathematica conversion function now take an ExpressIn, for composability, even if they don't use it.
  2. ExpressIn automatically applies to all types for which ToMathematica and related functions apply.
  3. All checks are compile-time, so if it compiles it's going to do the right thing.

@pleroy
Copy link
Member Author

pleroy commented Apr 26, 2020

retest this please

Sorry, something went wrong.

// The construction parameters must be for the base SI units. They may be in
// any order. If not enough parameters are specified for complete type erasure
// of the argument, compilation fails. An object with no template parameters
// may be used to indicate that type erasure should not happen.
Copy link
Member

Choose a reason for hiding this comment

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

The construction parameters must be for the base SI units.
The construction parameters must be values of distinct SI base quantities (or angle). They define a system of units.

If not enough parameters are specified for complete type erasure of the argument, compilation fails
if the other arguments of ToMathematica contain quantities that are not spanned by that system of units, the call to ToMathematica is ill-formed.

Sorry, something went wrong.

Copy link
Member

Choose a reason for hiding this comment

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

An object with no template parameters may be used to indicate that type erasure should not happen.

Why do we even have that lever?
This is not the logical extension of the behaviour with a nonempty parameter pack (that would be allowing only dimensionless entries), so it seems like it would bite us if we use it in a sufficiently-templated context (where the pack may or may not be empty at the callsite).
Beyound that admittedly theoretical concern, using ExpressIn<>{} to mean “use Quantity instead of expressing things in some system of units” feels about as readable as using negative values to denote an absent positive integer.

Use std::optional<ExpressIn<Qs...>> express_in = std::nullopt in ToMathematica, and give ExpressIn the semantic that follows from the definition (the dimensions of the argument must be in the span of the empty set, i.e., the arguments must be dimensionless).

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the comments.

Trust me, optional is where I started, and that doesn't work because:

  1. CTAD doesn't work in that case at the call site, and it becomes super-ugly to have to specify the template parameters as in ExpressIn<Angle, Length>(Degree, Metre).
  2. The check that we were given the default must be a constexpr if, to make sure that we don't try to compile the other branch of the if, which would not work because it doesn't have the units we need.

So we effectively need a "compile-time optional", here the special casing of ExpressIn<>. It's not like it is going to cause bugs anyway: if you write ExpressIn{} explicitly, you are mildly insane.

I'll add this this code is really at the bleeding edge of what the language can do, and large amounts of godbolting were needed to come up with this solution, and check that it works with the compilers involved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, as discussed offline.

std::is_same_v<Qs, Time> || std::is_same_v<Qs, Current> ||
std::is_same_v<Qs, Temperature> || std::is_same_v<Qs, Amount> ||
std::is_same_v<Qs, LuminousIntensity> || std::is_same_v<Qs, Angle>) ||
...));
Copy link
Member

Choose a reason for hiding this comment

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

This is
is_default ∨ ⋁Q∈QsB∈base quantities Q=B,
using a more common notation,
is_default ∨ ∃ Q∈Qs, ∃ B∈base quantities, Q=B,
which is
is_default ∨ ∃ Q∈Qs, Q∈base quantities.
You want
is_default ∨ ∀ Q∈Qs, Q∈base quantities,
which simplifies to
∀ Q∈Qs, Q∈base quantities,
which is
Q∈QsB∈base quantities Q=B,

  static_assert(
      ((std::is_same_v<Qs, Length> || std::is_same_v<Qs, Mass> ||
        std::is_same_v<Qs, Time> || std::is_same_v<Qs, Current> ||
        std::is_same_v<Qs, Temperature> || std::is_same_v<Qs, Amount> ||
        std::is_same_v<Qs, LuminousIntensity> || std::is_same_v<Qs, Angle>) && ...));

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I got the logic wrong (once more).

The fold expression that you write doesn't work with an empty pack, it would need a ... && true at the end.

More importantly, is_default is important for compilation performance: the default is used all over the place, and compilation is really slower if this condition cannot be short-circuited.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

static constexpr bool is_default = sizeof...(Qs) == 0;

// Check that only base SI units are specified. If a unit is specified
// multiple times, the calls to std::get in operator() won't compile.
Copy link
Member

Choose a reason for hiding this comment

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

Doing it that ways means that ToMathematica(1 * Metre, ExpressIn<Metre, Candela, Candela>) is legal, since that std::get only exists if the base quantity is used.
Let’s not depend on implementation details to check the contract; use the following:

template<typename... Ts>
constexpr bool all_different = false;
template<>
constexpr bool all_different<> = true;
template<typename T0, typename... Ts>
constexpr bool all_different<T0, Ts...> =
    ((!std::is_same_v<T0, Ts>) && ...) &&
    all_different<Ts...>;

Copy link
Member Author

@pleroy pleroy Apr 26, 2020

Choose a reason for hiding this comment

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

Good catch, done. Using the same trick with is_default for compilation speed.

file << Assign("barycentricPositions5",
ExpressIn(Metre, barycentric_positions_5_year));
ToMathematica(barycentric_positions_5_year, ExpressIn(Metre)));
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think the ToMathematica is needed, since you added express_in to Assign and the other things. Same elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@eggrobin eggrobin added the LGTM label Apr 26, 2020
@pleroy pleroy merged commit a403ffc into mockingbirdnest:master Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants