-
Notifications
You must be signed in to change notification settings - Fork 69
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
Conversation
retest this please |
mathematica/mathematica.hpp
Outdated
// 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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:
- 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)
. - 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, as discussed offline.
mathematica/mathematica.hpp
Outdated
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>) || | ||
...)); |
There was a problem hiding this comment.
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∈Qs
⋁B∈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∈Qs
⋁B∈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>) && ...));
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
mathematica/mathematica.hpp
Outdated
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. |
There was a problem hiding this comment.
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...>;
There was a problem hiding this comment.
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))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ExpressIn
, for composability, even if they don't use it.ExpressIn
automatically applies to all types for whichToMathematica
and related functions apply.