docs(ref): document `mp_units.core` #628

JohelEGP · 2024-10-18T20:44:33Z

This is still a WIP:

This is also blocked on
a number of minor points which I'll review later.

There are some TBDs, mainly for algorithms which need wording.
These are:

The quantity specification conversion algorithms.
The collapse common unit algorithms.
The formatting functions for dimensions and units.
The semantic requirements on the QuantityCharacterRepresentation concepts.
sudo_cast.

Work left (besides the TBDs above):

Text output.
Less priority:
- system_reference
- math.h
- random.h

mpusz · 2024-10-19T04:56:14Z

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).

mpusz

This is huge!!! Lots of great work! Thank you!!!

docs/api_reference/src/quantities.tex

mpusz · 2024-10-19T20:11:35Z

docs/api_reference/src/quantities.tex

How can we specify that some public identifiers should not be exported from the std module?

Reading https://wg21.link/std.modules, it seems like there's no way.

I will ask LWG members

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.

exposition-only is not a solution, as I wrote in the LWG reflector.

mpusz · 2024-10-19T20:17:56Z

docs/api_reference/src/quantities.tex

+\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 are some users that derive from quantity already. We support this in mp-units with derived_from. Should we remove this support?

Should we remove this support?

I've always been against it.
Do you have the related issue/PR numbers?
I hope to convince you to do that with my TBD review here.

This is controversial for me as well. In C++ we typically discourage people from inheriting from std types. It would be great if we could find a better solution here.

The most glaring issue is that for a parameter constrained with
std::derived_from<quantity> or std::derived_from<quantity_point>,
the implementation doesn't convert to the quantity base subobject.

For example, in operator+:

template<std::derived_from<quantity> Q, auto R2, typename Rep2> requires detail::CommonlyInvocableQuantities<std::plus<>, quantity, quantity<R2, Rep2>> [[nodiscard]] friend constexpr Quantity auto operator+(const Q& lhs, const quantity<R2, Rep2>& rhs) { using ret = detail::common_quantity_for<std::plus<>, quantity, quantity<R2, Rep2>>; const ret ret_lhs(lhs);

We need this conversion from Q, type of lhs, to ret,
to be the same as ret(static_cast<const quantity&>(lhs)).
The implementation doesn't do that (like as-variant),
so we need wording to require those parameter types
to not hide members
and prevent conversions like the above to have a different meaning.
Perhaps by requiring Q to behave as-if the base template in the specified wording
(e.g., by borrowing https://eel.is/c++draft/namespace.std#2.2).

I do not oppose removing the support for deriving from a quantity.

docs/api_reference/src/quantities.tex

mpusz · 2024-10-23T18:45:24Z

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?

Co-authored-by: Mateusz Pusz <[email protected]>

mpusz · 2024-10-24T05:31:45Z

I try to review in LatTeX as it is easier to provide comments that way, and I also need to learn the syntax.

docs/api_reference/src/quantities.tex

mpusz · 2024-10-25T14:43:01Z

docs/api_reference/src/quantities.tex

+  requires(sizeof...(Den) <= 1 && @\exposidnc{valid-ratio}@(Num, Den...) && @\exposidnc{positive-ratio}@(Num, Den...) &&
+           !@\exposidnc{ratio-one}@(Num, Den...))


Do you plan also to provide valid-ratio, positive-ratio, and ratio-one.

Or better.
This is part of:

This is also blocked on [...]
and a number of minor points which I'll review later.

For now, I fixed this with commit 6d64c98.

Co-authored-by: Mateusz Pusz <[email protected]>

docs/api_reference/src/quantities.tex

Co-authored-by: Mateusz Pusz <[email protected]>

docs/api_reference/src/quantities.tex

JohelEGP · 2024-11-25T01:21:15Z

docs/api_reference/src/quantities.tex

 \ref{qty.helpers}       & Helpers                                & \tcode{mp_units.core} \\
-\ref{qty.traits}        & Traits                                 & \\
-\ref{qty.concepts}      & Concepts                               & \\
-\ref{qty.types}         & Types                                  & \\
-\ref{qty.compat}        & Compatibility                          & \\
-\ref{qty.one}           & Dimension one                          & \\ \rowsep
+\ref{qty.expr.temp}     & Expression template                    & \\
+\ref{qty.dim}           & Dimension                              & \\
+\ref{qty.spec}          & Quantity specification                 & \\
+\ref{qty.unit.mag}      & Unit magnitude                         & \\
+\ref{qty.unit}          & Unit                                   & \\
+\ref{qty.ref}           & Reference                              & \\
+\ref{qty.rep}           & Representation                         & \\
+\ref{qty.traits}        & Quantity traits                        & \\
+\ref{qty}               & Class template \tcode{quantity}        & \\
+\ref{qty.pt.orig}       & Point origin                           & \\
+\ref{qty.pt}            & Class template \tcode{quantity_point}  & \\ \rowsep
 \ref{qty.systems}       & Systems                                & \tcode{mp_units.systems} \\
 \ref{qty.chrono}        & \tcode{std::chrono} compatibility      & \\


Right now, [qties] has many top-level subclauses (.18).
And we're still missing the specification of the module mp_units.system.
It currently beats the C++ Standard library Clauses', of which these have the most:
[support] (.13), [utilities] (.16), [algorithms] (.12), [time] (.15), [input.output] (.13), [re] (.12), and [thread] (.11).

I think we should try to leave the top-level subclauses
for the domain and programming entities
that are most indispensable for the general problem but not our particular solution.
That'd mean moving 5.6 "Expression template" under 5.5 "Helpers"
(though indispensable to our particular solution,
it's not for the general problem of a "quantities and units library").
Also moving 5.9 "Unit magnitude" under 5.10 "Unit"
(in the theory, a unit is a quantity chosen by convention.
The unit magnitude here is a programming concept for the compile-time benefits).

It should be possible to dissolve 5.13 "Quantity values".
quantity_values can be moved under 5.12 "Representation".
And if it isn't added under ::std, it can then be renamed to representation_values
(e.g., ::std::units::representation_values).
QuantityLike and QuantityPointLike can then be moved
next to quantity and quantity_point, respectively.
Also, shouldn't these concepts be exposition only?

Sure, "Unit magnitude" should be under "Unit".

Expression template probably as well. We may also need to rename it, as some people already objected to the use of this term for our purposes. The general-purpose expression template library would have totally different interfaces and rules. Ours is DSL (Domain Specific) version of expression templates.

Also, shouldn't these concepts be exposition only?

Initially, I thought it might be useful for a user to assert if a custom type satisfies those requirements. This is also why RepresentationOf still takes quantity_character explicitly (even though not used by the framework anywhere). If you think it is not a good enough reason to make it public, we can convert it to an exposition-only concept.

Expression template probably as well. We may also need to rename it, as some people already objected to the use of this term for our purposes. The general-purpose expression template library would have totally different interfaces and rules. Ours is DSL (Domain Specific) version of expression templates.

To me, it seems more like a computer algebra system.

It should be possible to dissolve 5.13 "Quantity values".
quantity_values can be moved under 5.12 "Representation".
And if it isn't added under ::std, it can then be renamed to representation_values
(e.g., ::std::units::representation_values).
QuantityLike and QuantityPointLike can then be moved
next to quantity and quantity_point, respectively.

You can already do this rename, though.

To me, it seems more like a computer algebra system.

I am not sure. It does not feel like a calculator 😉

How about 5.6 "Symbolic computations"?

JohelEGP · 2024-11-25T01:44:18Z

docs/api_reference/src/quantities.tex

+template<typename T, template<@\seebelow@> typename U>
+consteval bool @\exposidnc{is-derived-from-specialization-of}@();  // \expos


@mpusz Please, try replacing all the "is derived from specialization of" variable templates with overloaded function templates.

I tried, but it seems that clang-18 and below are broken 😢
https://godbolt.org/z/GPEMq6TKM

JohelEGP · 2024-11-25T15:14:03Z

docs/api_reference/src/quantities.tex

+  friend consteval @\exposidnc{ratio}@ operator/(@\exposidnc{ratio}@ lhs, @\exposidnc{ratio}@ rhs) { return lhs* @\exposidnc{ratio}@{rhs.den, rhs.num}; }
+};
+
+consteval bool is_integral(@\exposidnc{ratio}@ r) { return r.num % r.den == 0; }


Oh, were you referring to is_integral?
I thought we were talking about ratio.
is_integral is currently unused, as is common_ratio.
If those are not used after filling in the TBDs, they should be removed.

Otherwise, yeah, maybe they should be exposition only.

JohelEGP · 2024-11-25T16:32:01Z

docs/api_reference/src/quantities.tex

+Let \tcode{lhs} be \tcode{utf8} for the last two constructors,
+and otherwise:
+\begin{codeblock}
+std::bit_cast<std::fixed_u8string<N>>(std::basic_fixed_string(portable))


Hmm... I don't remember them changing the char type and its literals to be ASCII/UTF-8 compatible.
I just checked https://en.cppreference.com/w/cpp/language/types#Character_types
and https://en.cppreference.com/w/cpp/language/character_literal#Explanation.
The latter points to https://en.cppreference.com/w/cpp/language/charset#Code_unit_and_literal_encoding,
which hasn't changed since C++98 and still says

The ordinary and wide literal encodings are otherwise implementation-defined.

Anyways, I'll leave it as std::bit_cast.
Tell me if you'd prefer it to be changed to std::ranges::to.

JohelEGP · 2024-11-25T16:55:29Z

docs/api_reference/src/quantities.tex

+  requires @\seebelownc@
+struct @\libglobal{power}@ final {
+  using factor = F;
+  static constexpr @\exposidnc{ratio}@ exponent{Num, Den...};


I think exponent should be exposition only,
given that ratio is exposition only.
We should also make factor exposition only,
as we only care that the names per and power are not exposition only.

Sure, both factor and exponent should be exposition-only. We just want to reserve the class name so the implementations have a similar look and feel without mandating any particular implementation.

docs/api_reference/src/quantities.tex

JohelEGP · 2024-11-25T19:58:19Z

docs/api_reference/src/quantities.tex

+template<std::intmax_t Num, std::intmax_t Den, template<typename...> typename To,
+         typename OneType, template<typename, typename> typename Pred = @\exposidnc{type-less}@, typename T>
+  requires(Den != 0)
+consteval auto @\exposidnc{expr-pow}@(T);                    // \expos


Is it OK to use (Den != 0) instead of detail::non_zero<Den>?

Sure! Someone added those math concepts but I also consider them often more an issue than help. If you are OK with that, I can remove them from the code and replace with proper expressions.

mpusz requested changes Oct 19, 2024

View reviewed changes

JohelEGP force-pushed the ref_docs branch 5 times, most recently from 05ab0a8 to 8bf5db5 Compare October 23, 2024 16:15

This comment was marked as outdated.

Sign in to view

JohelEGP and others added 6 commits October 23, 2024 17:57

docs(ref): document mp_units.core

644b8d1

docs(ref): document mp_units.core

600a69b

Co-authored-by: Mateusz Pusz <[email protected]>

docs(ref): document mp_units.core

e819430

Co-authored-by: Mateusz Pusz <[email protected]>

docs(ref): document mp_units.core

9a9c343

Co-authored-by: Mateusz Pusz <[email protected]>

docs(ref): document mp_units.core

543254a

Co-authored-by: Mateusz Pusz <[email protected]>

docs(ref): document mp_units.core

d454ccc

Co-authored-by: Mateusz Pusz <[email protected]>

JohelEGP force-pushed the ref_docs branch from 8bf5db5 to d454ccc Compare October 23, 2024 22:12

docs(ref): fill [qty.ref]

60dd2cb

mpusz requested changes Oct 25, 2024

View reviewed changes

JohelEGP and others added 2 commits October 25, 2024 14:28

[mp.units.core.syn] Update pi's UCN

d9248b1

Co-authored-by: Mateusz Pusz <[email protected]>

[qty.unit.types] Update Magnitude * Unit

824f0c1

Co-authored-by: Mateusz Pusz <[email protected]>

mpusz requested changes Oct 25, 2024

View reviewed changes

docs/api_reference/src/quantities.tex Outdated Show resolved Hide resolved

docs/api_reference/src/quantities.tex Outdated Show resolved Hide resolved

docs/api_reference/src/quantities.tex Outdated Show resolved Hide resolved

JohelEGP and others added 8 commits October 25, 2024 18:43

[qty.dim.types] Fix derived_dimension example

8bce9c6

Co-authored-by: Mateusz Pusz <[email protected]>

[qty.spec.concepts] Update _QuantityKindSpec_

58487d6

Co-authored-by: Mateusz Pusz <[email protected]>

Merge branch 'master' into ref_docs

0cab71d

[mp.units.core.syn] Add \u03C0

1f6be11

[qty.spec.types] Improve description of kind_of

17585ce

[qty.unit.types] Fix named_unit description of "kind" parameter

9b35ad4

[qty.unit.types] Improve named_unit example comment

ae58e6c

Co-authored-by: Mateusz Pusz <[email protected]>

[qty.unit.types] Further improve named_unit example comments

14c0ee6

JohelEGP added 14 commits November 24, 2024 09:52

[qties] Update UnitMagnitude/unit_magnitude

59913be

[qties] Update to remove _SameQuantitySpec_

aa7a18d

[qty.spec] Update _QSProperty_

d14abc4

[qty.rep] Update representation concepts with CPOs

3b04b7e

[qty.cmp][qty.pt.cmp] Use _with concepts as appropriate

a8daee3

Merge branch 'master' into ref_docs

ae96518

[qty.unit.types] Specify the point origin argument

4e8fdab

ref(docs): document delta and point

2f4f4f2

docs(ref): Review [qty.helpers.non.types]

93b8607

[qty.compat.traits] Reword to avoid "quantity type"

6544b9f

docs(ref): Review [qty.ratio]

9c3526b

[qties] Remove explicit(false)

6eff5ed

docs(ref): Review [qty.sym.txt]

4012e2e

[qties] Order arguments of \indexlibrarymember conventionally

f5f4a75

mpusz previously approved these changes Nov 25, 2024

View reviewed changes

JohelEGP added 8 commits November 25, 2024 14:05

docs(ref): Review [qty.expr.temp]

45f183b

[qty.expr.temp.algo] Simplify _expr-fractions_

83cf005

[qty.expr.temp.algo] Improve requirements on Pred

1204fbd

[qty.expr.temp.algo] Add missing *elaborated-type-specifier*s

04f7c18

[qty.expr.temp.algo] Move description before declaration

09b3fca

[qty.expr.temp.algo] Update constraints of _expr-map_

b089dae

[qty.expr.temp.algo] Use "is of the form"

7195c84

[qty.expr.temp] Add missing index entries

e54a860

JohelEGP commented Nov 25, 2024

View reviewed changes

JohelEGP dismissed mpusz’s stale review via e54a860 November 25, 2024 18:56

JohelEGP added 3 commits November 25, 2024 15:01

[qties] Use indexlibraryglobalexpos

8f91cd3

docs(ref): review [qty.dim]

9ada586

[qty.expr.temp.algo] Remark error condition

b77f2bc

JohelEGP commented Nov 25, 2024

View reviewed changes

mpusz approved these changes Nov 26, 2024

View reviewed changes

		requires(sizeof...(Den) <= 1 && @\exposidnc{valid-ratio}@(Num, Den...) && @\exposidnc{positive-ratio}@(Num, Den...) &&
		!@\exposidnc{ratio-one}@(Num, Den...))

		template<typename T, template<@\seebelow@> typename U>
		consteval bool @\exposidnc{is-derived-from-specialization-of}@(); // \expos

docs(ref): document mp_units.core #628

Are you sure you want to change the base?

docs(ref): document mp_units.core #628

Conversation

JohelEGP commented Oct 18, 2024 • edited Loading

mpusz commented Oct 19, 2024

mpusz left a 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

This comment was marked as resolved.

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

mpusz commented Oct 23, 2024

This comment was marked as outdated.

mpusz commented Oct 24, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

docs(ref): document `mp_units.core` #628

docs(ref): document `mp_units.core` #628

JohelEGP commented Oct 18, 2024 •

edited

Loading