Skip to content

Units multiplied or divided by ratio #388

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

Closed
CPMOliveira opened this issue Jan 22, 2018 · 13 comments
Closed

Units multiplied or divided by ratio #388

CPMOliveira opened this issue Jan 22, 2018 · 13 comments

Comments

@CPMOliveira
Copy link
Contributor

It's not a issue, but a suggestion, all units remain its units when multiplied or divided by ratio.

@gojanpaolo
Copy link
Contributor

gojanpaolo commented Jan 22, 2018

I think the units already have generated operators (multiply or divide to double) for that. Or maybe I misunderstood your suggestion? :)

e.g. public static Length operator *(Length left, double right)

var lengthRatio = new Length() / new Length(); //double
Length finalLength = new Length() * lengthRatio;

@0xferit
Copy link
Contributor

0xferit commented Jan 23, 2018

What is not working that way at the moment? Perhaps an operator overload is missing unintentionally.

@CPMOliveira
Copy link
Contributor Author

CPMOliveira commented Jan 23, 2018

In fact, what happens is the operation, for example with Length and Ratio

var length = new Length() / new Ratio(); //Lenght
var length = new Length() * new Ratio(); //Lenght

@0xferit
Copy link
Contributor

0xferit commented Jan 23, 2018

I couldn't understand what you mean, can you please write down what you expect and what you actually get?

@gojanpaolo
Copy link
Contributor

I think he's suggesting to implement operator overload (* and /) for all the quantities (Length, Mass, etc) to the Ratio struct.

currently this is not possible:
var length = new Length() * new Ratio(); //compiler error

@CPMOliveira
Copy link
Contributor Author

CPMOliveira commented Jan 24, 2018

Exactly!

Thanks @gojanpaolo

@gojanpaolo
Copy link
Contributor

@CPMOliveira Could you provide a real world example on how this would be beneficial? I would actually prefer defining the specific Ratio property to relate to other units.

e.g.
Length length = new Length() * new Ratio().DecimalFractions;

...or maybe I'm just not using the Ratio struct that often. :)

@angularsen
Copy link
Owner

I'm a bit unsure about this request. I understand the usecase, Length * Ratio = Length. It absolutely makes sense, however I have some arguments against:

  • I rarely use Ratio and normally multiply/divide by a numeric value instead, I suspect this is the most typical use by our users - but I just speculate on that
  • If I understand correctly, you want all quantities to support Ratio multiplication and division operator overloads as an alternative to numeric values. This means 2 multiplication overloads and 1 division overload for all our 60+ quantities. As discussed in Proposal: Reducing size of library (WIP) #372 where we address the issue of library binary size, it turns out that many methods/properties with small bodies actually make up a significant part of the binary size, so I am very wary of adding code that increases linearly with our 60+ quantities (as in this case) and a lot more so for code that increases linearly with our 600+ units (not this case).

TLDR; I am not sure how much Ratio is used like this and I am a bit concerned of the increase in binary size.

@gojanpaolo
Copy link
Contributor

gojanpaolo commented Jan 24, 2018

@angularsen Another approach is to implement an implicit conversion from Ratio to double (and/or QuantityValue).

//Ratio.extra.cs
public partial struct Ratio
{
	//...
	public static implicit operator double(Ratio ratio) => ratio._decimalFractions;
}

This will make Length * Ratio = Length possible

@angularsen
Copy link
Owner

Interesting, I didn't originally think implicit cast would work here, but it sure does.

If we can agree that decimal fractions is THE intuitive choice for an implicit cast, then I think I could get behind this proposal. I am a little bit concerned that someone will be bitten by accidentally assigning the ratio to a double and getting something else than they expected.

Say, if they are working with parts per million numbers and constructing ratios from these, then accidentally assigning a ratio to a double would give them decimal fractions instead. I think it feels kind of weird for this to happen, and the only motivation for adding it is to avoid binary size increase to support arithmetic using Ratio instead of double. And we're not sure how many are using ratios like this to begin with. Are we adding value here or are we just complicating things?

I'm a little on the fence on this one... would like to have some more opinions on this first.

@CPMOliveira
Copy link
Contributor Author

CPMOliveira commented Jan 29, 2018

Guys

Sorry for the delay in answering, but I was absent these last few days.
I use this library to write research on water resources, more specifically hydrological models.
And an example could be an equation in which I have a dimensionless variable. In this way the use of the Ratio is essential.
I understand that the size of the library is a restriction, in which case the proposition of @gojanpaolo is perfect.

An addendum: although I can use the dimensionless variables as double, many of them are represented as percentage, ppm or ppt.

@gojanpaolo
Copy link
Contributor

@CPMOliveira, keep in mind that my proposed solution (implicit conversion) removes static typing when using the Ratio struct which is not always desirable. :)

@angularsen
Copy link
Owner

angularsen commented Feb 3, 2018

Say, if they are working with parts per million numbers and constructing ratios from these, then accidentally assigning a ratio to a double would give them decimal fractions instead. I think it feels kind of weird for this to happen, and the only motivation for adding it is to avoid binary size increase to support arithmetic using Ratio instead of double.

So after thinking a bit more on this, if we add implicit cast to double then decimal fraction is probably the most sane choice rather than percentage, parts per million etc. I still don't have a good overview of how easy it is to "accidentally" cast from Ratio to double and expect it to be something else than decimal fraction, but I can imagine it happen. So the thing to consider is how likely such accidents are to happen versus the increase in binary size of adding Ratio to all quantities' operator overload for multiplication and division to make such accidents impossible.

Let's also not forget @gojanpaolo 's example, I tried to choose some quantities that make sense to me:

Ratio substanceRatio = Ratio.FromPartsPerMillion(100);
Mass totalMass = Mass.FromTons(1);
Mass substanceMass = totalMass * substanceRatio; // does not work today, proposed in this issue
Mass substanceMass2 = totalmass * substanceRatio.DecimalFractions; // works today

Although it is nice to not have to call .DecimalFractions I do think it's not too bad either.

Trying to come up with an example of when maybe implicit cast to double can trip you up:

// Maybe someone forgot to rename the method when it was changed from double to Ratio
Ratio GetCo2ConcentrationInAtmospherePpm()
{
    return Ratio.FromPartsPerMillion(400);
}

// Maybe you expected value 400 (PPM) here? Now you've got 4e-4 instead (decimal fractions)
double co2ConcentrationPpm = GetCo2ConcentrationInAtmospherePpm();

I think refactoring like this is probably the most likely reason for getting the implicit cast wrong. One can argue that it's the coder's own fault, but I'm not a fan of running into quirks like this myself. Implicit casts should be used with care.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants