-
Notifications
You must be signed in to change notification settings - Fork 97
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
Implement muldiv assign for Ratio #364
Open
jacg
wants to merge
2
commits into
iliekturtles:master
Choose a base branch
from
jacg:muldiv-assign-ratio
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,197 @@ | ||
//! Tests summarizing which binary operations are implemented. | ||
//! | ||
//! This includes: | ||
//! | ||
//! + the operations implemented using the `impl_ops!` macro. | ||
//! + the comparison operators: `==`, `<`, `>`, `<=` and `>=`. | ||
//! | ||
//! The purpose is not to test the implementations exhaustively, but rather to | ||
//! give a clear overview of which ones are present or absent, depending on | ||
//! factors like: | ||
//! | ||
//! + `autoconvert` being enabled or disabled | ||
//! + whether the operands are values or refs | ||
|
||
// Helper functions for constructing a variety of Quantities without excessive | ||
// syntactic noise. | ||
#[allow(unused_macros)] | ||
mod concise { | ||
|
||
// Bring the units used by the constructors into a limited scope | ||
mod units { | ||
pub use crate::si::{ | ||
area::square_meter, | ||
frequency::hertz, | ||
length::meter, | ||
ratio::ratio, | ||
time::second, | ||
velocity::meter_per_second, | ||
}; | ||
} | ||
|
||
// Define a set of quantities with different base units, for testing of `autoconvert` | ||
pub mod cgs { | ||
ISQ!(crate::si, f32, (centimeter, gram, second, ampere, kelvin, mole, candela)); | ||
} | ||
|
||
/// Generate a module containing concise constructors for Quantities based | ||
/// on a given storage type, in order to allow writing pithy tests | ||
macro_rules! constructors { | ||
($storage_type:ident $($function:item)*) => { | ||
#[allow(unused)] | ||
pub mod $storage_type { | ||
use crate::si::$storage_type as si; | ||
use crate::tests::impl_ops::concise::cgs; | ||
use crate::tests::impl_ops::concise::units::*; | ||
type T = $storage_type; | ||
$($function)* | ||
} | ||
}; | ||
} | ||
|
||
/// Generate a function called NAME which returns QUANTITY by interpreting | ||
/// its argument as UNIT | ||
/// | ||
/// wrap!(NAME(QUANTITY) UNIT); | ||
macro_rules! wrap { | ||
($name:ident ($quantity:ty) $unit:ident ) => { | ||
pub fn $name(x: T) -> $quantity { | ||
<$quantity>::new::<$unit>(x) | ||
} | ||
}; | ||
} | ||
|
||
// Use the above macros to generate terse Quantity constructors. | ||
#[cfg(feature = "f32")] | ||
constructors! { f32 | ||
wrap!( m( si::Length) meter); | ||
wrap!( cgs_m(cgs::Length) meter); | ||
wrap!( s( si::Time) second); | ||
wrap!( one( si::Ratio) ratio); | ||
wrap!(cgs_one(cgs::Ratio) ratio); | ||
wrap!( m2( si::Area) square_meter); | ||
wrap!( mps( si::Velocity) meter_per_second); | ||
wrap!( hz(si::Frequency) hertz); | ||
} | ||
|
||
} | ||
|
||
// The tests will come in 4 varieties: `vv`, `vr`, `rv` & `rr`. with `r` and `v` | ||
// standing for 'value' and 'reference', describing the left and right operands | ||
// of the operator being tested. | ||
|
||
/// Generate a test that checks the result of a binary operation | ||
/// | ||
/// There are two different kinds of tests: | ||
/// | ||
/// 1. verify the result of a binary operation expression | ||
/// 2. verify the side effect of an assignment operator on the left operand | ||
/// | ||
/// Syntactically these look like | ||
/// | ||
/// 1. test!(TEST_NAME LHS OPERATOR RHS, EXPECTED); | ||
/// 2. test!(TEST_NAME \[ LHS \] OPERATOR RHS, EXPECTED); | ||
/// | ||
/// In other words, the side-effect test is distinguished from the expression | ||
/// test by wrapping the left operand (the one which will be mutated by the | ||
/// operator) in square brackets. | ||
/// | ||
/// Additionally, some tests should only be run when the `autoconvert` features | ||
/// is enabled. These are identified by using the `ac` token at the start of the | ||
/// input to `test!(...)`: | ||
/// | ||
/// 1. Run test always: `test!( ...)` | ||
/// 2. Run test only when `autoconvert` enabled: `test!(ac ...)` | ||
/// | ||
/// Furthermore, the `ac` variant of `test!` appends `_autoconvert` to | ||
/// TEST_NAME, in order to distinguish automatically between tests which require | ||
/// autoconvert and those that don't. | ||
#[allow(unused_macros)] | ||
macro_rules! test { | ||
// The first two arms (with `ac` as the first input token) forward the | ||
// request to the last two arms, after injecting two changes: | ||
// 1. add `#[cfg(feature = "autoconvert")]` | ||
// 2. append `_autoconvert` to the test name | ||
(ac $test_name:ident [$lhs:expr] $op:tt $rhs:expr, $expected:expr) => { | ||
paste::paste! { | ||
#[cfg(feature = "autoconvert")] | ||
test!([<$test_name _ autoconvert>] [$lhs] $op $rhs, $expected); | ||
} | ||
}; | ||
(ac $test_name:ident $lhs:expr, $rhs:expr) => { | ||
paste::paste! { | ||
#[cfg(feature = "autoconvert")] | ||
test!([<$test_name _ autoconvert>] $lhs, $rhs); | ||
} | ||
}; | ||
// ---------------------------------------------------------------- | ||
// test assignment operations: let mut l; l += r | ||
($test_name:ident [$lhs:expr] $op:tt $rhs:expr, $expected:expr) => { | ||
#[test] | ||
fn $test_name() { | ||
let mut x = $lhs; | ||
x $op $rhs; | ||
assert_eq!(x, $expected); | ||
} | ||
}; | ||
// test binary operator expressions: l + r | ||
($test_name:ident $lhs:expr, $rhs:expr) => { | ||
#[test] | ||
fn $test_name() { | ||
assert_eq!($lhs, $rhs); | ||
} | ||
}; | ||
} | ||
|
||
// The number in the comment after each test indicates which implementation | ||
// provides the functionality being tested, according to the table shown here: | ||
// https://github.com/iliekturtles/uom/pull/307#issuecomment-1186208970 | ||
#[rustfmt::skip] | ||
#[cfg(feature = "f32")] | ||
mod vv { | ||
use super::concise::f32::*; | ||
//#[cfg(feature = "autoconvert")] | ||
|
||
// NOTE: tests labelled with `ac` just inside `test!(` require the | ||
// `autoconvert` feature to be enabled. | ||
|
||
test!(ac add m(1.) + cgs_m(2.), m(3.)); // 1 | ||
test!(ac sub m(8.) - cgs_m(2.), m(6.)); // 1 | ||
test!( add m(1.) + m(2.), m(3.)); // 2 | ||
test!( sub m(8.) - m(2.), m(6.)); // 2 | ||
test!(ac add_assign [ m(1.) ] += cgs_m(2.), m(3.)); // 3 | ||
test!(ac sub_assign [ m(8.) ] -= cgs_m(2.), m(6.)); // 3 | ||
test!( add_assign [ m(1.) ] += m(2.), m(3.)); // 4 | ||
test!( sub_assign [ m(8.) ] -= m(2.), m(6.)); // 4 | ||
test!(ac mul m(4.) * cgs_m(2.), m2(8.)); // 5 | ||
test!(ac div m(6.) / cgs_m(2.), one(3.)); // 5 | ||
test!( mul m(4.) * m(2.), m2(8.)); // 6 | ||
test!( div m(6.) / s(2.), mps(3.)); // 6 | ||
test!( mul_ratio_left one(4.) * m(2.), m(8.)); // 6 | ||
test!( div_ratio_left one(6.) / s(2.), hz(3.)); // 6 | ||
test!( mul_ratio_right m(4.) * one(2.), m(8.)); // 6 c.f. AAA below | ||
test!( div_ratio_right m(6.) / one(2.), m(3.)); // 6 c.f. BBB below | ||
test!(ac mul_ratio_right m(4.) * cgs_one(2.), m(8.)); // 6 c.f. CCC below | ||
test!(ac div_ratio_right m(6.) / cgs_one(2.), m(3.)); // 6 c.f. DDD below | ||
test!( mul_assign_ratio [ m(4.) ] *= one(2.), m(8.)); // c.f. AAA above | ||
test!( div_assign_ratio [ m(6.) ] /= one(2.), m(3.)); // c.f. BBB above | ||
test!(ac mul_assign_ratio [ m(4.) ] *= cgs_one(2.), m(8.)); // c.f. CCC above | ||
test!(ac div_assign_ratio [ m(6.) ] /= cgs_one(2.), m(3.)); // c.f. DDD above | ||
test!( mul_bare_right m(4.) * 2. , m(8.)); // 7 | ||
test!( div_bare_right m(6.) / 2. , m(3.)); // 7 | ||
test!( mul_assign_bare [ m(2.) ] *= 3. , m(6.)); // 8 | ||
test!( div_assign_bare [ m(6.) ] /= 3. , m(2.)); // 8 | ||
test!( mul_bare_left 4. * m(2.), m(8.)); // 9 | ||
test!( div_bare_left 6. / s(2.), hz(3.)); // 9 | ||
|
||
test!( eq m(1.) == m(2.), false); | ||
test!(ac eq m(1.) == cgs_m(2.), false); | ||
test!( lt m(1.) < m(2.), true ); | ||
test!(ac lt m(1.) < cgs_m(2.), true ); | ||
test!( gt m(1.) > m(2.), false); | ||
test!(ac gt m(1.) > cgs_m(2.), false); | ||
test!( le m(1.) <= m(2.), true ); | ||
test!(ac le m(1.) <= cgs_m(2.), true ); | ||
test!( ge m(1.) >= m(2.), false); | ||
test!(ac ge m(1.) >= cgs_m(2.), false); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -263,6 +263,7 @@ mod a_struct { | |
} | ||
|
||
mod asserts; | ||
mod impl_ops; | ||
mod quantities; | ||
mod quantity; | ||
mod system; |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
autoconvert!
andnot_autoconvert!
versions are needed. Scroll up a bit in the file and see$MulDivTrait
for examples. Theautoconvert!
versions should usechange_base::<Dr, Ul, Ur, V>(&rhs.value)
. Thenot_autoconvert!
parameter should only have oneU
parameter.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.
Hmm, I had hoped to test everything that needs to be implemented. But I guess that the
not_autoconvert!
version is only needed when theautoconvert
feature is disabled, while the tests always run with theautoconvert
feature enabled. So the tests simply do not test thenot_autoconvert!
versions at all. Is that right?Ifs so, do you have any ideas about how to test the
not_autoconvert!
versions?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 full test suite has test runs with and without
autoconvert
. Most of the tests do operations where both quantities have the same base units and run for both. Then there are some that are only run whenautoconvert
is enabled and ensure that operations with different base units work as expected.Test cases:
uom/.github/workflows/ci-full-test-suite.yml
Lines 41 to 51 in f8caf07
Tests:
uom/src/tests/quantity.rs
Lines 54 to 70 in f8caf07
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.
TLDR
Apoogies for being dense and verbose. It seems that I still don't understand what
autoconvert
does.My understanding is that the
autoconvert
feature enables binary operations to act on differentUnit
s within the sameQuantity
. In other words, withoutautoconvert
operations likesomething in metres + something in kilometres
should not work.The behaviour of the code appears to contradict my understanding.
Gory details
I've added an
ac
switch to thetest!
macro, so thatIn the last commit I've pushed (entitiled:
REVERT ME: run 'ac' tests even when autoconvert not enabled
) I have commented out the feature gate, so that theac
tests run even ifautoconvert
is not enabled. At this point, I would expect the six tests labelled withac
inhttps://github.com/jacg/uom/blob/eef466b82d0969b3063582108fcb9100d0563ba2/src/tests/impl_ops.rs#L135-L144
... which for some reason doesn't get expanded in the GH interface, so I'll paste the relevant lines by hand:
... so, I expect those six test marked with
ac
(just insidetest!(
) either to FAIL or to FAIL TO COMPILE, if theautoconvert
feature is not enabled. I expect this, because they all use differentUnit
s (belonging to the sameQuantity
) on their LHS/RHS. Specificallyac add
usesm
on the LHS andkm
on the RHS: these are bothLength
s, but have differentUnit
s. According to my understanding this should only work ifautoconvert
is enabled.And yet, they not only compile but they also run and pass, when
autoconvert
is disabled:Points to note:
--no-default-features
--features "f32 si"
does NOT includeautoconvert
_autoconvert
run and pass.Where is my error / misunderstanding / brain fart?
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.
autoconvert
affects how quantities with different base units work. Auom
Quantity
doesn't store the original unit (e.g. meter, kilometer, ...) that was used. Instead the value is normalized into a base unit. Meter by default in the case of length. When talking about operations between quantities we can be more explicit by saying a length (theD
generic parameter of quantity) stored as meters (theU
generic parameter of quantity) inf32
(theV
generic parameter of quantity).You're never adding units together, always quantities.
autoconvert
affects how operations are defined such that whenautoconvert
is not enabled you can only operate on quantities with the same base units. e.g. length stored as meters can only operate with length stored as meters, but not length stored as kilometers. Whenautoconvert
is enabled this limitation is lifted.Compare https://github.com/iliekturtles/uom/blob/master/examples/si.rs and https://github.com/iliekturtles/uom/blob/master/examples/base.rs.
si.rs
will work regardless of theautoconvert
setting because the same base units are used for all quantities.base.rs
requiresautoconvert
to be enabled because a new set of base units (using centimeter, gram, second) is 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.
Thanks for your patience and your clear explanations. Your description is consistent with my initial understanding of
autoconvert
. I'm not sure why, in the process of working on this, I convinced myself that it was something else. Apologies.I have corrected my confusion, rebased, and force-pushed two commits:
Add concise tests summarizing which binops work
Implement MulDivAssign with Ratio on RHS
The first of these commits summarizes which arithmetic binary operations are implemented, both in the presence and absence of
autoconvert
. I have also added some tests for the comparison operators. For now, I'm only testing value operands; the idea is to add ref-ref, value-ref, and ref-value versions of these tests, for use in #307.The second of these commits is the implementation that I pushed earlier, without any changes. Before this commit, these four tests :
fail to compile (which is why they are commented out in the first commit). After the second commit, all four of these tests pass (which is why they are uncommented in the commit).
The first two of these tests use the
si
system thatuom
provides out of the box on both the LHS and the RHS. As such they should work both with and withoutautoconvert
. The second two tests differ from the first two in that they use a set ofcgs
units on the RHS; thus they should only work whenautoconvert
is enabled. All this is borne out in practice.Returning to what you said right at the top of this conversation:
I was hoping that these tests would demonstrate that this is the case. Instead, they seem to be indicating that the single implementation I provided, covers both cases.
So ...
I'll try to implement both versions when I find some more time, but I just wanted to check in and make sure that I'm getting back on the right path.
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.
Perhaps I'm misunderstanding, but it seems to me that
change_base
applies a conversion factor which is appropriate for conversion between corresponding quantities in the two different sets of units, but in this case the RHS isRatio
while the LHS is anything butRatio
. In my specific test, this results in applying the conversion factor of 100 which converts betweencm
andm
, but this is inappropriate, because the RHS isRatio
.Also, is it possible to have a 'base unit' for
Ratio
which differs from 1? I see howISQ
can create sets of units with arbitrary base units for the 7 fundamental quantities, but I don't see howRatio
's base unit could be anything other than 1.If the above points are approximately correct, then it would seem that separate
autoconvert!
andnot_autoconvert!
implementations are not necessary ... just like in theQuantity */= f32
case.