-
Notifications
You must be signed in to change notification settings - Fork 86
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
docs(ref): document mp_units.core
#628
base: master
Are you sure you want to change the base?
Conversation
This is great! Thanks! Regarding magnitudes, please note that they are meant to be implementation-defined. Only a few user-facing API things are public (construction helpers (mag, mag_ratio, and mag_power), concept, operators, magnitude<...> where ... is implementation-defined). |
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 huge!!! Lots of great work! Thank you!!!
template<typename T> | ||
concept quantity_like = @\seebelownc@; |
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.
We probably should be consistent and use either PascalCase everywhere or refactor all the concepts to standard_case somehow. You already have some concepts in PascalCase above.
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.
Yeah, I should be using what the master
branch uses, at least for the public API.
Whenever you see a snake_case
concept, that's just something I did at first.
As I start documenting their surroundings, I'll be changing them back to PascalCase
.
|
||
\begin{itemdecl} | ||
template<typename T> | ||
concept @\defexposconceptnc{tag-type}@ = type_is_empty(^T) && type_is_final(^T); // \expos |
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.
Why not in terms of is_empty
and is_final
?
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.
How can we specify that some public identifiers should not be exported from the std
module?
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.
Reading https://wg21.link/std.modules, it seems like there's no way.
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 will ask LWG members
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.
Dropping in mid-stream here -- is there a problem with marking them as exposition only? That basically says -- we're going to act like this thing exists in the standard, but we might implement it differently or call it something different.
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.
exposition-only is not a solution, as I wrote in the LWG reflector.
\label{term.quantity.type} | ||
A \defnadj{quantity}{type} | ||
is a type \tcode{\placeholder{Q}} | ||
that is a specialization of \tcode{quantity} or \tcode{quantity_point}. |
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.
There are some users that derive from quantity already. We support this in mp-units with derived_from. Should we remove this support?
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.
Also, I would not account quantity_point to a quantity type. One can do quantity equations on quantity types, which is not true for quantity points.
Putting them into the same basket makes it also harder to communicate.
\begin{itemdecl} | ||
template<typename T> | ||
concept @\deflibconcept{Quantity}@ = @\unspecnc@; |
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.
Why this is unspecified?
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.
It's not done, but is needed by their uses (the use of \libconcept
requires a corresponding \deflibconcept
).
\begin{itemdecl} | ||
template<typename T> | ||
concept @\deflibconcept{point_origin}@ = @\unspecnc@; |
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.
Why this is unspecified?
A \defnadj{quantity point}{type} is a specialization of \tcode{quantity_point}. | ||
Let \tcode{\placeholder{Q}} be a quantity point type. | ||
\tcode{\placeholdernc{Q}::point_origin} represents | ||
the origin point of a position vector\irefiev{102-03-15} |
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.
"position vector" is only one of many quantities specified in ISQ. Please do not use this name here. It is even hard to talk about position vectors for masses or voltage.
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.
It sounds like you think this refers to the quantity.
This refers to yet another definition for the term.
The context is seen in "Area" here: https://www.electropedia.org/iev/iev.nsf/display?openform&ievref=102-03-15.
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.
Yes, I thought so. But it seems a bit ambiguous. Maybe we can find some alternative spelling?
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.
They only have that term for that definition.
There may be a more inclusive definition for the point origin.
Perhaps we could use the definition of point space.
Something like Q::point_origin
represents the point whose vector is the neutral element for addition.
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.
...which doesn't work for relative point origins.
05ab0a8
to
8bf5db5
Compare
Could you please not squash or rebase the branch? It is impossible to track what changes between commits this way. I need to start reviewing all 2500 lines from scratch as I do not know what has changed. Maybe provide something for merge and open another PR with additional changes? |
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: Mateusz Pusz <[email protected]>
Co-authored-by: Mateusz Pusz <[email protected]>
Co-authored-by: Mateusz Pusz <[email protected]>
Co-authored-by: Mateusz Pusz <[email protected]>
Co-authored-by: Mateusz Pusz <[email protected]>
I try to review in LatTeX as it is easier to provide comments that way, and I also need to learn the syntax. |
template<@\exposconceptnc{DerivedQuantitySpecExpr}@... Expr> | ||
struct @\exposidnc{derived-quantity-spec-impl}@ : | ||
@\exposidnc{quantity-spec-interface}@, | ||
@\exposidnc{expr-fractions}@<struct dimensionless, Expr...> { |
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.
For this, I had a "chicken and egg problem". This is why I used is_dimensionless
instead. Did you manage to workaround this issue and use dimensionless directly here?
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.
No, I had no idea.
But the C++ standard specification is not code.
Implementors are expected to make it work,
just like how we use names introduced later without a forward declaration.
But I suppose there'd be less friction
if we avoided that for this implementation detail.
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 see where a complete type for the OneType
is needed
in the mp-units implementation of expr_fractions
.
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 am not sure what you mean. It is used here:
if constexpr (size == 2 && OneType<type_list_front<List>>::value) |
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.
Well, yes, that's called OneType
.
I mean the actual type representing the neutral element.
In these cases, one
, dimension_one
, and 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.
I never provided those in my implementation. It was your design change 😉
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 reference documentation doesn't need the OneType
to be complete.
Even if it did, maybe it's OK:
But the C++ standard specification is not code.
Implementors are expected to make it work,
just like how we use names introduced later without a forward declaration.
I also don't see where you need a complete type in mp-units.
So maybe it was a problem in the past, but it wouldn't be anymore.
concept @\defexposconceptnc{PowerVBase}@ = (^T == ^int) || ^T == ^std::intmax_t || @\libconcept{MagConstant}@<T>; | ||
|
||
template<typename T> | ||
concept @\defexposconceptnc{MagnitudeSpecExpr}@ = @\exposconceptnc{PowerVBase}@<T> || @\unspecnc@; |
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.
What is the second argument of the conjunction here?
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.
It's power_v
in mp-units.
It's not in a detail::
namespace,
but it seems to be the indirection used for the prime factorization
(a reason users should use mag
and not magnitude
, IIUC).
This ties to what I mention in the opening comment:
Additionally, some things might be missing with regards to magnitude.
Like implementation defined limits,
or what else can we expect or require from an implementation.
constexpr bool @\exposidnc{is-per-of-units}@<per<Ts...>> = (... && (@\libconcept{Unit}@<Ts> || @\exposidnc{is-power-of-unit}@<Ts>)); | ||
|
||
template<typename T> | ||
concept @\defexposconceptnc{DerivedUnitExpr}@ = @\libconcept{Unit}@<T> || @\exposidnc{is-power-of-unit}@<T> || @\exposidnc{is-per-of-units}@<T>; |
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 am wondering if we should provide those in the synopsis (for all derived entities). After the discussion on the LWG reflector, it seems we can't make it invisible in a module, but people say that stating that instantiating derived class templates by a user is IFNDR is fine.
With the above, we do not officially need to check for the correctness of this expression. I did those mostly to check my own implementation, but now, I am considering removing that for compile-time performance reasons.
Please note that the checks are not exhaustive because we do not check if Ts...
are in proper order.
Do you think it makes sense to keep them to improve the understanding of the library at the cost of many more lines in the spec? We will probably need to describe how to instantiate those types in expr-XXX
functions anyway.
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.
In general, I've been wondering whether even the library should name these directly.
Preferably, they'd be named trough an indirection
that either requires the argument to be correct (e.g., in order, simplified)
or makes them correct and also simplifies them.
For example, your recent fix with commit 9c7d3b0
would be simpler if there was an alias template scalar_unit_t
that performed those simplifications.
One problem with the former approach of requiring correct arguments
is the lack of portability when naming those templates.
For example, magnitude<4>
isn't the same as mag<4>
's in mp-units.
And mp-units prime factorization is an implementation detail,
so the answer for other implementations isn't guaranteed to be the same.
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 derived_XXX
, per
, power
, magnitude
, and power_v
class templates are instantiated by the framework only. The framework does it correctly, and we have tests for that. We might not need to provide constraints that check if arguments are of the specified type.
I do not want to provide any indirection here, as a user simply should not do it. IFNDR is a good solution, and we should probably mention this in our wording for such types.
Regarding magnitudes, there is no framework to instantiate them. That is why we provide a mag
factory to properly calculate and provide the arguments.
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.
From the above, I assume you are OK with removing those containing concepts?
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.
Yes, that'd be great.
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
constexpr bool space_before_unit_symbol = true; | ||
|
||
template<> | ||
inline constexpr bool @\libspec{space_before_unit_symbol}{one}@<one> = false; |
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.
one
is not declared before this point. Maybe this specialization should be put after one
definition?
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.
See above's
But the C++ standard specification is not code.
Implementors are expected to make it work,
just like how we use names introduced later without a forward declaration.
\end{itemdescr} | ||
|
||
\begin{itemdecl} | ||
consteval bool @\exposidnc{is-convertible-to-base-subobject-of}@( // \expos |
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.
Can we use is-derived-from-specialization-of here? "convertible" may mean more than inheritance here, and the std::derived_from
concept already requires public inheritance, so we do not have to mention convertibility in the name.
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.
Definitely.
I was going to do that,
after finishing the quantity types,
when moving away from reflection.
|
||
friend consteval @\exposidnc{ratio}@ operator*(@\exposidnc{ratio}@ lhs, @\exposidnc{ratio}@ rhs); | ||
|
||
friend consteval @\exposidnc{ratio}@ operator/(@\exposidnc{ratio}@ lhs, @\exposidnc{ratio}@ rhs) { return lhs* @\exposidnc{ratio}@{rhs.den, rhs.num}; } |
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.
friend consteval @\exposidnc{ratio}@ operator/(@\exposidnc{ratio}@ lhs, @\exposidnc{ratio}@ rhs) { return lhs* @\exposidnc{ratio}@{rhs.den, rhs.num}; } | |
friend consteval @\exposidnc{ratio}@ operator/(@\exposidnc{ratio}@ lhs, @\exposidnc{ratio}@ rhs) { return lhs * @\exposidnc{ratio}@{rhs.den, rhs.num}; } |
requires(sizeof...(Den) <= 1 && @\exposidnc{valid-ratio}@(Num, Den...) && @\exposidnc{positive-ratio}@(Num, Den...) && | ||
!@\exposidnc{ratio-one}@(Num, Den...)) |
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.
Do you plan also to provide valid-ratio
, positive-ratio
, and ratio-one
.
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.
Or better.
This is part of:
This is also blocked on [...]
and a number of minor points which I'll review later.
Co-authored-by: Mateusz Pusz <[email protected]>
Co-authored-by: Mateusz Pusz <[email protected]>
} | ||
}; | ||
|
||
struct @\exposidnc{quantity-spec-interface}@ : @\exposidnc{quantity-spec-interface-base}@ { |
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.
quantity-spec-interface-base
is not needed here. I did it only for older compilers where quantity-spec-interface
is a template, which would complicate implementing QuantitySpec
and result in ambiguity when operators would be called with a different type.
Here, quantity-spec-interface
is not a template, so it may contain all of the implementation, and the QuantitySpec
concept may use it in its definition.
Co-authored-by: Mateusz Pusz <[email protected]>
Co-authored-by: Mateusz Pusz <[email protected]>
Co-authored-by: Mateusz Pusz <[email protected]>
constexpr bool @\exposidnc{is-per-of-units}@<per<Ts...>> = (... && (@\libconcept{Unit}@<Ts> || @\exposidnc{is-power-of-unit}@<Ts>)); | ||
|
||
template<typename T> | ||
concept @\defexposconceptnc{DerivedUnitExpr}@ = @\libconcept{Unit}@<T> || @\exposidnc{is-power-of-unit}@<T> || @\exposidnc{is-per-of-units}@<T>; |
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
If the first argument is a base quantity dimension, | ||
then \tcode{Q} is that base quantity\irefiev{112-01-08}. | ||
If an argument is a quantity calculus\irefiev{112-01-30} \placeholder{C}, | ||
then $\tcode{Q} = \placeholder{C}$ (a quantity equation\irefiev{112-01-31}). |
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 am not sure it Q = C
statement is correct. If C = 1 / time
, then what Q
should be? It might be frequency, activity, bit rate, etc. Maybe it is better to state that Q
is implicitly convertible from C
(with C
being its recipe)?
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.
Yeah.
It might be better to avoid using equations.
Those don't account for other properties of the involved quantities.
then \tcode{Q} is of its kind\irefiev{112-01-04}. | ||
|
||
\pnum | ||
\tcode{set} is the set of the numerical value of \tcode{Q}. |
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.
Today, it is not a set, and I am not sure it will ever be modeled as such. Each quantity has an exact character in ISQ. In engineering, it might be different. For example, it is common to represent acceleration as a directed scalar (minute means slowing down), which is not equivalent to norm(acceleration)
.
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.
It's this set: https://www.electropedia.org/iev/iev.nsf/display?openform&ievref=102-01-02.
I prefer a defined term than "character".
\pnum | ||
\tcode{set} is the set of the numerical value of \tcode{Q}. | ||
If not specified, then it defaults to \tcode{scalar} for the first signature, | ||
and to that of the quantity argument that comes before the \tcode{set} otherwise. |
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 do not understand this statement.
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.
Did you mean that the character is derived from the provided quantity for other signatures (if not set explicitly)?
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.
Also, we should probably state what happens when a user provides a character that conflicts with the character of the provided quantity. For now, it is possible, but I believe that when I finally have time to finish my work on vectors, tensors, and complexes, then the second and fourth signatures will not get a character at all (always derived from the calculus).
\tcode{set} is the set of the numerical value of \tcode{Q}. | ||
If not specified, then it defaults to \tcode{scalar} for the first signature, | ||
and to that of the quantity argument that comes before the \tcode{set} otherwise. | ||
\tcode{is_kind} specifies \tcode{Q} to start a new hierarchy tree of a kind. |
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.
New pnum?
\end{itemdescr} | ||
|
||
\rSec1[qty.traits]{Traits} | ||
\rSec3[qty.set.traits]{Set} |
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.
Set might never be true (as we use only one character for a quantity type), and even if for some reason it will be true, still, I think it is not the best name here. I would prefer "character" to be consistent with enum and other places where use this term.
The term "character" was taken from this ISO quote: https://mpusz.github.io/mp-units/latest/users_guide/framework_basics/character_of_a_quantity/#characters-dont-apply-to-dimensions-and-units
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.
That's not in the 2022 edition.
Although it still exists at https://www.electropedia.org/iev/iev.nsf/display?openform&ievref=112-01-11.
template<typename T> | ||
concept @\defexposconceptnc{Scalable}@ = | ||
@\exposconceptnc{CastableNumber}@<T> || | ||
(@\exposconceptnc{CastableNumber}@<@\exposidnc{actual-value-type-t}@<T>> && | ||
@\exposconceptnc{ScalableNumber}@<T, std::common_type_t<@\exposidnc{actual-value-type-t}@<T>, std::intmax_t>>); |
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.
It would be good if you could look into this concept and possibly improve it.
\label{term.quantity.type} | ||
A \defnadj{quantity}{type} | ||
is a type \tcode{\placeholder{Q}} | ||
that is a specialization of \tcode{quantity} or \tcode{quantity_point}. |
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.
Also, I would not account quantity_point to a quantity type. One can do quantity equations on quantity types, which is not true for quantity points.
Putting them into the same basket makes it also harder to communicate.
using namespace si; | ||
|
||
if constexpr (is_same_v<Period, std::chrono::nanoseconds::period>) | ||
return nano<second>; | ||
else if constexpr (is_same_v<Period, std::chrono::microseconds::period>) | ||
return micro<second>; | ||
else if constexpr (is_same_v<Period, std::chrono::milliseconds::period>) | ||
return milli<second>; | ||
else if constexpr (is_same_v<Period, std::chrono::seconds::period>) | ||
return second; | ||
else if constexpr (is_same_v<Period, std::chrono::minutes::period>) | ||
return minute; | ||
else if constexpr (is_same_v<Period, std::chrono::hours::period>) | ||
return hour; | ||
else if constexpr (is_same_v<Period, std::chrono::days::period>) | ||
return day; | ||
else if constexpr (is_same_v<Period, std::chrono::weeks::period>) | ||
return mag<7> * day; | ||
else | ||
return mag_ratio<Period::num, Period::den> * second; |
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.
We can probably do it with prose. Also, it should be part of the SI paper, not P3045.
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.
Very well.
It's already done,
and is part of mp-units' reference documentation.
template<@\exposconceptnc{DerivedQuantitySpecExpr}@... Expr> | ||
struct @\exposidnc{derived-quantity-spec-impl}@ : | ||
@\exposidnc{quantity-spec-interface}@, | ||
@\exposidnc{expr-fractions}@<struct dimensionless, Expr...> { |
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 never provided those in my implementation. It was your design change 😉
Co-authored-by: Mateusz Pusz <[email protected]>
This is still a WIP:
This is also blocked on #626 and #627,
and a number of minor points which I'll review later.
There are some TBDs, mainly for algorithms which need wording.
These are:
is-positive-integral-power
/is_positive_integral_power
,due to the fact that
power_v
is an unspecified implementation detail.Additionally, some things might be missing with regards to magnitude.
Like implementation defined limits,
or what else can we expect or require from an implementation.
Work left: The quantity types.