-
Notifications
You must be signed in to change notification settings - Fork 79
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
quantity: add definite constraint on q* "scalar" fixes https://github… #116
Conversation
src/include/units/quantity.h
Outdated
@@ -324,7 +324,7 @@ template<typename D, typename U1, typename Rep1, typename U2, typename Rep2> | |||
|
|||
template<typename D, typename U, typename Rep, Scalar Value> | |||
[[nodiscard]] constexpr Quantity AUTO operator*(const quantity<D, U, Rep>& q, const Value& v) | |||
requires std::regular_invocable<std::multiplies<>, Rep, Value> | |||
requires std::regular_invocable<std::multiplies<>, Rep, Value> && std::is_arithmetic_v<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.
is_arithmetic
cannot be used here because it limits values only to built-in arithmetic types and not to all types that behave as such. Besides, not all arithmetic operations are needed in this case so this heavily overconstrains the function template.
|
Add a DimensionlessQuantity concept - multiplication by itself does not affect its dimension. Add an Arithmetic concept - the built in arithmetic types are models. Make Arithmetic models of DimensionlessQuantity. quantity: Constrain the ambiguous multiply ops to multiplication by DimensionlessQuantity.
So just call it units::DimensionlessQuantity and stay low ;-)
So you need to add a test case that shows how that 'overconstraint' causes a problem, becasue currently (EDIT :) all tests are passing , at least locally for me. Have a Nice Day ;-) |
@@ -322,18 +322,18 @@ template<typename D, typename U1, typename Rep1, typename U2, typename Rep2> | |||
return ret(ret(lhs).count() - ret(rhs).count()); | |||
} | |||
|
|||
template<typename D, typename U, typename Rep, Scalar Value> | |||
template<typename D, typename U, typename Rep, DimensionlessQuantity 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.
Did you mean Arithmetic here? I do not think that in 2m * 2
a 2
is a dimensionless quantity... It is just a regular value/multiplier/ratio or however you want to call it (if not a scalar).
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 Arithmetic here?
No I didnt. T : model of DimensionlessQuantity requires only
T x ;
Dimensionless y = x * x;
(note: eliding division for now)
Usually the following is true, e.g for arithmetic
T x ;
x = x * x;
( but not necessarily which allows dimensionless ratios such as radian and steradian)
I do not think that in
2m * 2
a2
is a dimensionless quantity...
If you are asking : is the 2 in the above a DimensionlessQuantity?, Yes it is a dimensionless quantity , plugging the above requirement to an integer
int x = 2;
int y = x * x; // works fine
Scalar is a different concept to DimensionlessQuantity. A Scalar has magnitude but not direction
https://www.britannica.com/science/scalar
c++ arithmetic type is a model of Scalar and a model of DimensionlessQuantity
units::quantity with a Scalar Rep type is a model of Scalar but not a model of DimensionlessQuantity.
You could examine the rep type of the units::quantity to find if the quantity is a Scalar. If the Rep type is a scalar, then the units::quantity is a scalar, but If the Rep type is a vector or a matrix then clearly the quantity isn't a Scalar
src/include/units/concepts.h
Outdated
@@ -289,4 +289,21 @@ concept Scalar = | |||
std::regular<T> && | |||
(detail::constructible_from_integral<T> || detail::not_constructible_from_integral<T>); | |||
|
|||
template <typename T> | |||
concept Arithmetic = std::is_arithmetic_v<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.
As I wrote many times already we cannot constrain the library to fundamental types only. There are a lot of type-safe wrappers around fundamental types that have to work nicely here as well.
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.
As I wrote many times already we cannot constrain the library to fundamental types only. There are a lot of type-safe wrappers around fundamental types that have to work nicely here as well.
There are 3 UDT math types I can find in current standard library with binary math operators
std::complex
std::chrono::duration
std::valarray
In each of those types binary operator functions are constrained in both their arguments ( Take a close look at how they are implemented rather than just the documentation). There is no precedent for your statement about unconstrained arguments in the standard library.
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.
Types of https://www.boost.org/doc/libs/release/libs/safe_numerics/ and those being worked on by the WG21's Numerics SG should work as well. Like how std::chrono::duration
's rep
isn't limited to fundamental types.
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.
Types of https://www.boost.org/doc/libs/release/libs/safe_numerics/ and those being worked on by the WG21's Numerics SG should work as well. Like how
std::chrono::duration
'srep
isn't limited to fundamental types.
Sure.
We seem to be back to the Numeric
concept at this point
https://www.boost.org/doc/libs/1_73_0/libs/safe_numerics/doc/html/numeric.html
So Here is an examination of how to do DimensionlessQuantity in a structural way, but allowing customisation ( e.g for the angle case)
Bear in mind I dont use Concepts much so it can no doubt be done better.
Anyway it seems to show that it can be made to work if the will is there ;-)
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.
Linked here (#27 (comment)) is QuantityRatio
, an interesting test case for DimensionlessQuantity
Sorry, I missed all in my answer. Fixed it now. |
Add a DimensionlessQuantity concept - multiplication by itself does not affect its dimension. Add an Arithmetic concept - the built in arithmetic types are models. Make Arithmetic models of DimensionlessQuantity. quantity: Constrain the ambiguous multiply ops to multiplication by DimensionlessQuantity.
international volume : Add barrel volume unit for quantity_ratio example
…wikius/units into kwikius_scalar_vector_multiply
… is causing fail" This reverts commit 92347ea.
This reverts commit f8c74e0.
…t fixes fail, locally
quantity: add definite constraint on q* "scalar" fixes #88.
So the
is_arithmetic
constraint is too strong as is, rather should be anumeric
concept ( allowing UDT number types as well), but showing that a multiply function needs strong constraints to avaoid ambiguity.How to decide what to define in your librray?
Generally is the result in your domain e.g quantity * numeric -> quantity. but quantity * vector -> vector so that is the domain of vector. And behold it works ; -)