angularsen / UnitsNet

Makes life working with units of measurement just a little bit better.
https://www.nuget.org/packages/UnitsNet/
MIT No Attribution
2.18k stars 379 forks source link

Oktas #879

Closed MarkCiliaVincenti closed 3 years ago

MarkCiliaVincenti commented 3 years ago

I want to add oktas to UnitsNet. The problem is that they're usually expressed in integers. What approach do you recommend? I was thinking internally saving a Ratio and then express in octa integers when converted.

https://en.wikipedia.org/wiki/Okta

ebfortin commented 3 years ago

An Okta is always an integer value? You can't have fraction of an Okta? If that's the case then use the current backing store with a max of 9 and a min of 9 and always round the value at creation.

MarkCiliaVincenti commented 3 years ago

An Okta is always an integer value? You can't have fraction of an Okta? If that's the case then use the current backing store with a max of 9 and a min of 9 and always round the value at creation.

But what if someone does an Okta.FromPercent and gets an instance of Okta which is saving an integer okta value, and the developer wants to convert it back to the original percentage value? This is what I mean.

angularsen commented 3 years ago

Hi, how about something like this:

This way it rounds to the nearest Okta integer, while quantity supports values like 35% cloud cover or fractions. Should probably also use System.Math.Clamp to prevent values outside the valid range.

Source: https://en.wikipedia.org/wiki/Cloud_cover

MarkCiliaVincenti commented 3 years ago

Brilliant, I have this CloudCover.json so far, not sure what else I need. Some notes:

1) I used System.Math.Round(x / 0.125, MidpointRounding.AwayFromZero) as I prefer it to the default rounding method 2) What about unit tests? 3) How do we define the ranges [0..8] etc?

{
  "Name": "CloudCover",
  "BaseUnit": "Fraction",
  "XmlDoc": "Cloud cover (also known as cloudiness, cloudage, or cloud amount) refers to the fraction of the sky obscured by clouds when observed from a particular location.",
  "Units": [
    {
      "SingularName": "Okta",
      "PluralName": "Oktas",
      "FromUnitToBaseFunc": "x * 0.125",
      "FromBaseToUnitFunc": "System.Math.Round(x / 0.125, MidpointRounding.AwayFromZero)",
      "Localization": [
        {
          "Culture": "en-US",
          "Abbreviations": [ "oktas" ]
        }
      ]
    },
    {
      "SingularName": "Percent",
      "PluralName": "Percent",
      "FromUnitToBaseFunc": "x/1e2",
      "FromBaseToUnitFunc": "x*1e2",
      "Localization": [
        {
          "Culture": "en-US",
          "Abbreviations": [ "%" ]
        }
      ]
    },
    {
      "SingularName": "Fraction",
      "PluralName": "Fractions",
      "FromUnitToBaseFunc": "x",
      "FromBaseToUnitFunc": "x",
      "Localization": [
        {
          "Culture": "en-US",
          "Abbreviations": [ "" ]
        }
      ]
    }
  ]
}

Committed to https://github.com/MarkCiliaVincenti/UnitsNet/tree/cloudcover for now.

angularsen commented 3 years ago

MidpointRounding.AwayFromZero

Fine by me.

What about unit tests?

They are auto-generated, you just need to fill in the test value for each unit. See https://github.com/angularsen/UnitsNet/wiki/Adding-a-New-Unit.

How do we define the ranges [0..8] etc?

This is a first, but how about wrapping everything in a System.Math.Clamp? It won't look pretty, but it should do the job.

Alternatively we could throw an exception, but I'm not sure if that currently fits into how the code generator currently inserts these conversion functions into C# code, might require some modifications.

angularsen commented 3 years ago

Looks like you are on the right path though, please create a PR and we can continue the discussion there and have some actual code to look at.

MarkCiliaVincenti commented 3 years ago

So I can't use System.Math.Clamp as it's not in netstandard2.0, nor can I use Math.Min(Math.Max()) as the 'x' in Math.Max is replaced with the value, nor is using both Min and Max efficient.

Ideally we write our own extension method. Something like this:


public static T Clamp<T>(this T val, T min, T max) where T : IComparable<T>
{
    if (val.CompareTo(min) < 0) return min;
    else if(val.CompareTo(max) > 0) return max;
    else return val;
}
MarkCiliaVincenti commented 3 years ago

Also, what should I do to change type? Okta should be in byte or at least int32.

angularsen commented 3 years ago

Ah, right. Yes, an extension would be a good approach then.

You can't change the type of the public IQuantity members, they use double as the common numeric type, but here are some options to consider:

  1. Add a UnitsNet/CustomCode/Quantities/CloudCover.extra.cs with additional members:

    public int IntegerOktas => Convert.ToInt32(Oktas);
  2. Extend the code generator and JSON schema to specify a different numeric type to use for Okta unit and in particular for the int Oktas { get; } property and FromOktas(int oktas) factory method. I'm not yet sure if this is a good idea or not, but it should be doable. The reason I am hesitant is that we have had quite some pain due to letting certain quantities use decimal as the internal number type, while most use double - in particular for serialization and code generator edge cases this required a lot of extra checks and mental burden. I don't see any immediate problems with allowing this per unit, however.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.