Skip to content
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

Oktas #879

Closed
MarkCiliaVincenti opened this issue Dec 26, 2020 · 10 comments
Closed

Oktas #879

MarkCiliaVincenti opened this issue Dec 26, 2020 · 10 comments

Comments

@MarkCiliaVincenti
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Owner

angularsen commented Dec 28, 2020

Hi, how about something like this:

  • Add quantity CloudCover with units Okta [0..8], Percent [0..100] and Fraction [0..1] and the latter as the base unit (double numeric value).
  • Okta FromUnitToBaseFunc: x * 0.125
    • 0 Okta => 0 fractions
    • 1 Okta => 0.125 fractions
    • 2 Okta => 0.25 fractions
    • etc..
  • Okta FromBaseToUnitFunc: System.Math.Round(x / 0.125)
    • 0 fractions => 0 Okta
    • 0.15 fractions => 1.2 => 1 Okta
    • 0.2 fractions => 1.6 => 2 Okta
    • etc

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
Copy link
Contributor Author

MarkCiliaVincenti commented Dec 29, 2020

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@angularsen
Copy link
Owner

angularsen commented Dec 29, 2020

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);
  1. 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
Copy link

stale bot commented Mar 5, 2021

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.

@stale stale bot added the wontfix label Mar 5, 2021
@stale stale bot closed this as completed Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants