Preserve value and unit

[angularsen]

Motivation

Preserve the original value and unit and add getter properties Value and Unit to expose them.
Use Unit in ToString() instead of the base unit representation, to better match the units the user is working with.

This is a significant rewrite of how values are stored and how conversions are performed in quantity types. It is also the first baby steps of introducing some common values in base types #371 .

There should be no breaking changes in neither main lib nor JSON serialization.

Changes

Using example of Length, but this applies to all quantities:

• Change private field _meters to _value (still double or decimal, as before)
• Add private field LengthUnit? _unit
• Add Value getter prop (double or decimal)
• Add LengthUnit Unit getter prop, with fallback to base unit LengthUnit.Meter (1)
• Ctors without unit param assumes BaseUnit, backwards compatible
• Change ToString() to use Unit instead of DefaultToStringUnit, new test cases
• Mark DefaultToStringUnit as obsolete
• Add support for preserving decimal precision in QuantityValue (2)
• Defer conversion to base unit until calling properties like .Centimeters (3)
• Serialize Value+Unit instead of base unit and base unit value, backwards compatible, update test cases
• Make internals of UnitsNet visible to Json to reuse reflection abstraction code
• Rename variables in JSON serialization lib (use "quantity" term instead of "unit")

1. Default ctor (struct) and ctors without unit param assumes base unit, to be backwards compatible. We might want to change this in the future to require explicitly specifying the unit.
2. Needed to avoid precision issues going via double to construct decimal values.
3. Getting the value of unit properties is now twice as slow, as it first converts from Unit to base unit, then to target unit. Before this change, the first conversion was initially made when constructing the quantity. However, there are optimizations in-place to avoid converting if Unit already matches either base unit or the target unit.

Gotchas

• Serialization output changed (different value and unit for same quantity)
• In Preserve value and unit #389 (comment) I discuss the choice to not go with breaking change with regards to unspecified unit, but why we may want to do so later to require explicitly specifying the unit

[angularsen]

Some compile errors still to fix, maybe hold off reviewing until fixed.

[gojanpaolo]

@angularsen 💯 This is an awesome feature.

Though, I'm curious to why the need to remove the constructors using just a number without the Unit? It's intuitive enough to make the BaseUnit be the default Unit if nothing is specified.

[angularsen]

@gojanpaolo I guess you spotted a breaking change 😇

However, I really don't like constructors taking a value and assuming the consumer just knows what the base unit is or reads it from the xmldoc. We have even changed base units for quantities at some point, so I would prefer to have the consumer explicitly state what unit to construct with - just as we do with the FromMeters() methods. It is very clear what you are creating.

Going with that thought, instead of removing the ctor it should be obsoleted and removed later.

Update: Ok, seems I already did obsolete and not remove any ctors just yet. I'm still open to keep them if people find it intuitive to specify just the numeric value.

[angularsen]

It seems to compile now and is ready for review.

[gojanpaolo]

@angularsen Here's a couple that might seem to be an unexpected change in behavior compared to current release.

1. GetHashCode returns False when Equals(...) is True

var length1 = Length.FromMeters(1);
var length2 = Length.FromCentimeters(100);
bool isTrue = Equals(length1, length2);
bool isFalse = length1.GetHashCode() == length2.GetHashCode(); //True in current release

2. Default constructor of quantities produces invalid instances

double throwsException = new Length().Meters; //does not throw exception in current release

[angularsen]

@gojanpaolo Thanks!

GetHashCode returns False when Equals(...) is True

I don't consider this a breaking change. GetHashCode() is intended for hash sorts such as when using a quantity as the key in a dictionary entry. This should not be a behavior people depend on being fixed between versions, the main thing is that it should be reliable and consistent at runtime (not be based on mutable values for instance) - and it is.

Default constructor of quantities produces invalid instances

Sigh.. not sure exactly how to work around this. Struct don't allow us to specify the default values when using the default ctor. Another argument for class I guess.. Any ideas? We could have some fallback logic in the code that accesses the Unit value internally, so that it will always have some sane default (BaseUnit), but I'm not sure if it will give unpredictable behavior for the consumers.

Another option is to rethink this design and only store the base unit value as before, and store the constructed Unit for the purpose of using that as the default for ToString(). You do lose some precision since the values will be unnecessarily converted back and forth, but it wouldn't be any worse than it is today.

[gojanpaolo]

@angularsen

For the default constructor, I'm also thinking of a fallback logic. something like:

//Length
private readonly LengthUnit? _unit;
public LengthUnit Unit => _unit ?? BaseUnit;

Another approach is to change the default Unit enum values to the BaseUnit instead of Undefined.

public enum LengthUnit
{
	Undefined = -1,
	Meter = 0,
	Centimeter,
	//...
}

[angularsen]

Right, the reason I'm on the fence of using nullables as fallback is that then we're back to storing things on the heap and adding pressure to the garbage collector. I think at that point we could just as well go for class.

Changing default from Undefined is interesting, a breaking change though. How about instead treating Undefined implicitly as BaseUnit for the purpose of conversions? It does kind of violate its design, where DID want things to complain if you incorrectly constructed stuff without specifying the unit, but right now that behavior is causing problems.

[gojanpaolo]

treating Undefined implicitly as BaseUnit for the purpose of conversions

Should an exception be thrown if Undefined is passed on the constructor parameter because it might be weird to get something like this:

var length = new Length(1, LengthUnit.Undefined);
double cm = length.Centimeters; //100

Looking back at the suggestions, I think this (below) is the better solution.

only store the base unit value as before, and store the constructed Unit for the purpose of using that as the default for ToString()

...that is until/unless we move from struct to class which may have better solutions.

[dmf07]

Great work on this library! I have a need for maintaining the initial value and not necessarily the unit, although it makes sense to do that as well.

I don't think it makes sense for using Undefined as BaseUnit for conversions. Undefined is just that, Undefined, and should not guarantee a conversion. If you are going to use Undefined as the BaseUnit you might as well get rid of Undefined.

I think it would be OK to throw an error for Undefined Units, but currently that seems like a breaking change if a User was using the default behavior of the struct. I personally think you should construct your Unit of measure explicitly.

[angularsen]

@dmf07 I fully agree with all you said. I would not want anyone explicitly setting Undefined, that should throw.

However, the key problem is that we cannot implement code for the default ctor of structs. So, what should happen here in these examles? My intuitive answers in comments beside each, since Meter is the base unit of Length.

new Length().Value; // 0
new Length().Unit; // LengthUnit.Meter
new Length().ToString(); // "0 m"
new Length().ToString(LengthUnit.Centimeter); // "0 cm"

Assuming my intuition is valid here, we can achieve this by making the backend field of Unit property LengthUnit? as @gojanpaolo proposed earlier. See more about Nullable<T> below as I learned something new today.

Then Unit property becomes

public LengthUnit Unit => _unit.GetValueOrDefault(BaseUnit);

since we know it is ONLY the default struct ctor that could ever result in unit not being set. We also know it will never be Undefined and cause problems with conversions, since that is no longer the default and we throw for that value in other ctors.

Thoughts?

Nullable

So I assumed Nullable<T> would be a reference type and live on the heap and add GC pressure, but today I learned it is actually a struct and implemented pretty much this way; a value T and a bool for whether it has been set or not. Whether it lives on the stack or the heap is up to the CLR and depends on whether T is a value or reference type and if a value type it depends on the scope/lifetime of the value, as with any other value type.

Good answer from Eric Lippert himself
Source of Nullable.cs

[angularsen]

@dmf07 Just curious, what is your motivation for preserving the original value?

[dmf07]

My motivation for preserving the original value is primarily for user input without having a separate value to keep track of.

As far as using Nullable for the backing value of the Unit, that could fix the issue of initializing the struct without context, but I am under the opinion that you shouldn't use a measurement without knowing explicitly what unit of measure it is. The Length example 0 length typically works in all cases.
But what do you pick for a base unit in temperature? 0 Kelvin? And what happens when you calculate the difference in temperature between one set to the default and one explicitly set? Granted users of the library have to due their due diligence. It's the difference between a bad calculation and no calculation, I would rather error on the side of caution and not give false information.

[angularsen]

Good point about temperature, where 0 Kelvins does not equal 0 Fahrenheit. In that case, yes, I would intuitively assume 0 Kelvins. If other users share that intuition in all scenarios? Probably not.

I get your point about staying on the conservative path and throw instead of trying to assume some meaningful unit when none was specified.

I think our options regarding default ctor boil down to this:

1. Treat default ctor as Value = 0 and Unit = BaseUnit and allow to use the quantity as normal
   OR
2. Throw exception on pretty much all methods and properties when Unit == Undefined, except
   2.1 Value returns 0
   2.2 Unit returns Undefined
   2.3 Parameterless ToString() either returns "0 undefined" or throws. If we throw, then we should add DebuggerDisplay attribute to aid in debugging.

I agree with you that option 2 is probably the better choice. This PR is currently implemented pretty much like this already, but not the ToString() special case. It is still a breaking change, for those who rely on the behavior of the default ctor.

Thoughts? Any other options?

[angularsen]

Pushed more commits that update serialization to serialize using the constructed value+unit pair, as well as supporting the new QuantityValueDecimal type introduced in this PR.
All tests are now green.

[angularsen]

Some cleanup

[angularsen]

Thanks for the heads up @JKSnd, no problem!

[eriove]

I'm terribly sorry for not checking this in detail at an earlier stage.

Things I think are worth mentioning (so that they aren't overlooked but conscious choices):

• The behavior of ToString() is very different when values are created with the operator overloads. Since almost all of these use the SI constructors anything that passes through an overload will be printed in SI. I don't have any good solution to this, it is mainly an observation.
• The size of the struct has increased by an int. That means more memory copying every time a quantity is passed as an argument. It is probably still small enough for it not to matter.
• The code for creating a new instance of a quantity is significant longer. Again I have no idea what this means in practice. Before it was very likely to be inlined. I don't think this will be the case after this change.

I will try to run some of our performance test/benchmarks with this code and see what the difference is.

I think that my use case is quite far from the typical so I'm not sure if that really matters for the change.

Impressed by the code quality for such a large commit.

[angularsen]

Thanks for taking a look, your insight is always appreciated especially in light of any performance/memory impact. Benchmarks would be awesome, I have done very little of that at least to make an informed decision.

• SI units with operator overloads: I'm fine with this, I don't see any other behavior that would be more natural at this point.
• Size of struct: Right, this PR increases from 64 bits (double) to 64+32 bits (double + int). It might be an idea to go with byte (8 bit) or ushort (16 bit) backing type for the enum instead? We are nowhere near 256 units for any quantity yet, and can always bump it when we reach that.
• Longer code to create quantity: Could you please elaborate? Are you referring to the method call stack of Length.FromMeters(1) or are you referring to the use of QuantityValue type with implicit casts and it now having both a double? and decimal? field? In the former case, creation should be more light-weight now (no initial conversion to base unit). In the latter case, yes the QuantityValue type is getting a lot heavier now (128 bit decimal+ 2 HasValue bits). I don't like that type at all, but it does solve a real problem with binary size...

[eriove]

After running the code with our internal projects I have the following notes:

• Performance seems to be decreased by 10% in our current common use cases which seems reasonable. I would like to run some more test for some other pieces of code. But then I have to write the tests and I do not think I have that time.
• The assembly is no longer CLS compliant. I believe this is an old regression. I can make a pull request and add a unit test for that if you'd like
• The code size is twice of 3.69.0 (which was the one I used before, the breaking change to 3.70.0 will hold us from updating until we really need more units). It matters less to me personally but since we wanted to keep it small before it seems large. I haven't checked the size change since the previous version though.

I believe this is on the limit of what could be seen as a minor version, the underlying changes are large.

Again I want to stress that this is a great effort and that I'm sorry that I'm not contributing more.

[eriove]

Based on the performance tests I don't think we have to worry about most of my points. After discussing in Gitter we agreed to merge.

[angularsen]

Nuget 3.92 on the way out.