Implement muldiv assign for Ratio

[jacg]

As discussed here

[iliekturtles]

autoconvert! and not_autoconvert! versions are needed. Scroll up a bit in the file and see $MulDivTrait for examples. The autoconvert! versions should use change_base::<Dr, Ul, Ur, V>(&rhs.value). The not_autoconvert! parameter should only have one U parameter.

[jacg]

Hmm, I had hoped to test everything that needs to be implemented. But I guess that the not_autoconvert! version is only needed when the autoconvert feature is disabled, while the tests always run with the autoconvert feature enabled. So the tests simply do not test the not_autoconvert! versions at all. Is that right?

Ifs so, do you have any ideas about how to test the not_autoconvert! versions?

[iliekturtles]

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 when autoconvert 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

	- name: Test si
	uses: actions-rs/cargo@v1
	with:
	command: test
	args: --verbose --no-default-features --features "f32 si"
	- name: Test all non-storage type features
	uses: actions-rs/cargo@v1
	with:
	command: test
	args: --verbose --no-default-features --features "autoconvert f32 si use_serde"

Tests:

uom/src/tests/quantity.rs

Lines 54 to 70 in f8caf07

	#[cfg(feature = "autoconvert")]
	quickcheck! {
	#[allow(trivial_casts)]
	fn add(l: A<V>, r: A<V>) -> TestResult {
	let km: V = <kilometer as crate::Conversion<V>>::coefficient().value();
	let f = (&*l + &*r) / &km;
	let i = (&*l / &km) + (&*r / &km);
	if !Test::approx_eq(&i, &f) {
	return TestResult::discard();
	}
	TestResult::from_bool(
	Test::approx_eq(&k::Length::new::<meter>(&*l + &*r),
	&(k::Length::new::<meter>((*l).clone())
	+ f::Length::new::<meter>((*r).clone()))))
	}

[jacg]

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 different Units within the same Quantity. In other words, without autoconvert operations like something 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 the test! macro, so that

test!(ac ... etc)  // runs only when autoconvert feature enabled
test!(   ... etc)  // runs always

In 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 the ac tests run even if autoconvert is not enabled. At this point, I would expect the six tests labelled with ac in

https://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:

    test!(ac add                   m(1000.)   +     km(2.),     m(3000.)); // 1
    test!(ac sub                   m(8000.)   -     km(2.),     m(6000.)); // 1
    test!(   add                   m(   1.)   +      m(2.),        m(3.)); //  2
    test!(   sub                   m(   8.)   -      m(2.),        m(6.)); //  2
    test!(ac add_assign          [ m(1000.) ] +=    km(2.),       km(3.)); //   3
    test!(ac sub_assign          [ m(8000.) ] -=    km(2.),       km(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(4000.)   *     km(2.),      km2(8.)); //     5
    test!(ac div                   m(6000.)   /     km(2.),    ratio(3.)); //     5

... so, I expect those six test marked with ac (just inside test!() either to FAIL or to FAIL TO COMPILE, if the autoconvert feature is not enabled. I expect this, because they all use different Units (belonging to the same Quantity) on their LHS/RHS. Specifically ac add uses m on the LHS and km on the RHS: these are both Lengths, but have different Units. According to my understanding this should only work if autoconvert is enabled.

And yet, they not only compile but they also run and pass, when autoconvert is disabled:

cargo test impl --no-default-features --features "f32 si"                                                                                                                                                                                                                                                                                                                                 ─╯
   Compiling uom v0.33.0 (/home/jacek/rust/uom)
    Finished test [unoptimized + debuginfo] target(s) in 3.92s
     Running unittests src/lib.rs (target/debug/deps/uom-2f2c30298b0ddf3b)

running 28 tests
test tests::impl_ops::vv::add ... ok
test tests::impl_ops::vv::add_assign ... ok
test tests::impl_ops::vv::div_assign_bare ... ok
test tests::impl_ops::vv::div ... ok
test tests::impl_ops::vv::add_assign_autoconvert ... ok
test tests::impl_ops::vv::div_assign_ratio_p ... ok
test tests::impl_ops::vv::add_autoconvert ... ok
test tests::impl_ops::vv::div_assign_ratio ... ok
test tests::impl_ops::vv::div_autoconvert ... ok
test tests::impl_ops::vv::div_bare_left ... ok
test tests::impl_ops::vv::div_bare_right ... ok
test tests::impl_ops::vv::div_ratio_left ... ok
test tests::impl_ops::vv::div_ratio_right ... ok
test tests::impl_ops::vv::div_ratio_right_p ... ok
test tests::impl_ops::vv::mul ... ok
test tests::impl_ops::vv::mul_assign_bare ... ok
test tests::impl_ops::vv::mul_assign_ratio ... ok
test tests::impl_ops::vv::mul_assign_ratio_p ... ok
test tests::impl_ops::vv::mul_autoconvert ... ok
test tests::impl_ops::vv::mul_bare_left ... ok
test tests::impl_ops::vv::mul_bare_right ... ok
test tests::impl_ops::vv::mul_ratio_left ... ok
test tests::impl_ops::vv::mul_ratio_right ... ok
test tests::impl_ops::vv::mul_ratio_right_p ... ok
test tests::impl_ops::vv::sub ... ok
test tests::impl_ops::vv::sub_assign ... ok
test tests::impl_ops::vv::sub_assign_autoconvert ... ok
test tests::impl_ops::vv::sub_autoconvert ... ok

test result: ok. 28 passed; 0 failed; 0 ignored; 0 measured; 174 filtered out; finished in 0.00s

Points to note:

• --no-default-features
• --features "f32 si" does NOT include autoconvert
• The 6 tests whose names end in _autoconvert run and pass.

Where is my error / misunderstanding / brain fart?

[iliekturtles]

My understanding is that the autoconvert feature enables binary operations to act on different Units within the same Quantity. In other words, without autoconvert operations like something in meters + something in kilometers should not work.

autoconvert affects how quantities with different base units work. A uom 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 (the D generic parameter of quantity) stored as meters (the U generic parameter of quantity) inf32 (the V generic parameter of quantity).

let m = Length::new::<meter>(50.0); // Internally stored as 50.0 meters.
let km = Length::new::<kilometer>(50.0); // Internally stored as 50_000.0 meters.

You're never adding units together, always quantities. autoconvert affects how operations are defined such that when autoconvert 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. When autoconvert 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 the autoconvert setting because the same base units are used for all quantities. base.rs requires autoconvert to be enabled because a new set of base units (using centimeter, gram, second) is defined.

[jacg]

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:

1. Add concise tests summarizing which binops work
2. 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 :

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

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 that uom provides out of the box on both the LHS and the RHS. As such they should work both with and without autoconvert. The second two tests differ from the first two in that they use a set of cgs units on the RHS; thus they should only work when autoconvert is enabled. All this is borne out in practice.

Returning to what you said right at the top of this conversation:

autoconvert! and not_autoconvert! versions are needed.

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

• Have I misunderstood something again?
• If not, why are the two versions needed? Perhaps because, even though the single implementation is a correct implementation covering both cases, it is unnecessarily inefficient in (at least) one of the cases?

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.

[jacg]

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 is Ratio while the LHS is anything but Ratio. In my specific test, this results in applying the conversion factor of 100 which converts between cm and m, but this is inappropriate, because the RHS is Ratio.

Also, is it possible to have a 'base unit' for Ratio which differs from 1? I see how ISQ can create sets of units with arbitrary base units for the 7 fundamental quantities, but I don't see how Ratio's base unit could be anything other than 1.

If the above points are approximately correct, then it would seem that separate autoconvert! and not_autoconvert! implementations are not necessary ... just like in the Quantity */= f32 case.