Skip to content

Quantity Types with only a single unit #435

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
chrisnbray opened this issue May 5, 2018 · 13 comments
Closed

Quantity Types with only a single unit #435

chrisnbray opened this issue May 5, 2018 · 13 comments

Comments

@chrisnbray
Copy link

Unless I am missing something, there are a number of conversions that can have a value of only 1:1 because there is only one entry in the list of available conversions. An example would be QuantityType.ElectricCharge, which has only undefined and Coulomb as available units. Clearly if I am correct then until more units are added this quantity type is rendered entirely pointless.

At the moment there appears to be no easy way to work out whether or not to include each QuantityType in the list, i.e. to exclude those where conversion is not currently possible and thereby reduce pointless clutter in any UI.

For the moment I have simply added a 'Can Convert' method:

    public static bool CanConvert(QuantityType quantity)
    {
        Type unitEnumType = UnitEnumTypes.First(t => t.FullName == $"UnitsNet.Units.{quantity}Unit");
        return Enum.GetValues(unitEnumType).Length > 2;
    }

The question is, is there a better way than this to filter the list of quantity types to only those which are capable of converting something?

@angularsen
Copy link
Owner

angularsen commented May 5, 2018

Hi, I assume this is the context of some sort of dynamic unit conversion app similar to our sample app;
for a selected unit, show all units it can convert to?

In that context, you are right there is nothing built-in to help filter this. What you do is perfectly fine I think. One improvement could be to let UnitsNet provide a mapping like this:

image

Such a method could be added to our static, generated UnitSystem.g.cs class, usin our powershell code generator scripts. I would be happy to assist you if you want to pursue this.

The reason single-unit quantity types exist is to provide semantics for those types of values in an application with its own type, instead of passing double values around. We do, however, try to avoid such quantities since an application can easily create such types themselves. The ones we still add are either very common (ElectricCharge) or there are more units that can be added to it later if anyone needs it, maybe someone needs to represent Megacoulumb? We have generally taken the stance to only add units that people actually need instead of adding every possible unit in the multiverse.

Hope this helps!

@chrisnbray
Copy link
Author

I am sorry if I was unclear on the purpose. You are correct. I have created in Winforms a similar app to your WPF sample, and made it available to my staff to play with. They were of the opinion that it was pointless having entries with only a single unit, because it can do nothing other than clutter the UI.

Because there is the chance that other units and quantity types will be added in the future, we definitely do not want to use any sort of hard coded switch statement such as shown in your code. The problem is that it would have to be rearchitected for each addition, which is hardly ideal I am sure you will agree. The code I provided has the major advantage that it does not need to be changed no matter how many units or quantity types there are in the library. For this reason I am not in favour of your proposed solution.

Here is how I currently populate the QuantityType list:

    private void LoadQuantityTypes()
    {
        var quantitytypes = EnumHelper.ToList(typeof(QuantityType));
        IList<KeyValuePair<Enum, string>> filteredQuantitytypes = new List<KeyValuePair<Enum, string>>();

        // Strip out the undefined entry, and any entries that do not have the ability to perform any conversion
        foreach (var kvp in quantitytypes)
        {
            var quantityType = (QuantityType)kvp.Key;
            if (quantityType > QuantityType.Undefined && UnitConversionHelper.CanConvert(quantityType))
            {
                filteredQuantitytypes.Add(kvp);
            }
        }

        _view.SetQuantityTypesDataSource(filteredQuantitytypes);
    }

This does not care what content the library has, it will function regardless. However, I would have preferred not to need to iterate the enum to retrieve the entries I want to include.

It works perfectly as it is, I just wondered if there was a better way :-)

@angularsen
Copy link
Owner

angularsen commented May 5, 2018

Ah, sorry for not being clear myself. My proposal was intended for how to implement it in UnitsNet library, using our powershell code-generator scripts to generate the switch statement since it will always include the latest units for every build.

The advantage is that consumers don't need to mess with reflection (which may break with a future release of UnitsNet) and have a simple UnitsNet.UnitSystem.GetUnits(quantityType) method to get all LengthUnit values when passing in QuantityType.Length - or any other quantity added in the future.

@chrisnbray
Copy link
Author

In that case, it would definitely be better for the consumer if that were added, and it would have the advantage of removine the need for reflection as you say, but it would still leave us having to iterate to find out which QuantityTypes are capable of offering any form of conversion. The ideal solution would be something like:

    private void LoadQuantityTypes()
    {
        var quantityTypes = QuantityType.ActiveQuantityTypes();
        _view.SetQuantityTypesDataSource(quantityTypes);
    }

I understand that this is not available, and probably not practical given your architecture, but you can see that it would be easy for the consumer.

@angularsen
Copy link
Owner

This particular method I think belongs in an application. It is an application-specific need to know that a certain quantity only has a single unit for the purpose of filtering them. Also it is trivial to code this part so I don't think we will add it to UnitsNet. Enumerating the units, however, does have some value in including in the library and I'm happy to help you bring it into a pull request if you care to pursue that.

@chrisnbray
Copy link
Author

I am not sure I entirely agree with your comments, but I certainly understand and respect them. For me, being able to retrieve only valid or usable objects contained within it seems like the responsibility of the library and not the application accessing it which should just be able to say to the library 'give me only the things I can actually use' - not least because the ones with a single unit are useless in any environment with our without a UI - but I can see why you consider it differently. It is also worth noting that currently if you pass in QuantityType.Undefined during your iteration you get an exception rather than a graceful exit, and with your method we could pass back an empty array rather then throwing an exception assuming that is good practice as far as your library is concerned.

It would be interesting to pursue the unit enumeration though (noting the TODO note in the sample application), and I will certainly try to fit it into my schedule. If you can give me some guidance it would be appreciated as I am entirely new to github having initially joined purely so that I could ask this question!

@angularsen
Copy link
Owner

angularsen commented May 5, 2018

And I respect you being able to disagree in a good way :-)

I don't mean to be a stick in the mud, my intention is only to keep the library small, focused and solve a specific purpose - represent physical quantities and convert between units of a quantity. We also struggle a bit with binary size, so I hesitate to add anything I consider unnecessary.

  1. CanConvert(QuantityType quantity) is to me application-specific, or at least a very narrow usecase. I think it's better for the API to instead return the quantity's units, so you can count them, enumerate or whatever makes sense for your application and use. CanConvert is in my mind too limiting and also easy to do yourself.

  2. GetUnits(QuantityType quantity) should exclude Undefined units, I think. That value is only intended to help catch bugs where you accidentally did not assign a value, such as deserializing data. This is also why we throw if you try to do anything with Undefined - it is not supposed to be used.

  3. With my updated proposal your CanConvert() becomes:

    public static bool CanConvert(QuantityType quantity) => UnitSystem.GetUnits(quantity).Length > 1;
  1. I just realized I already made this implementation in WPF sample's UnitHelper.cs, which seems you already found.
    ///     TODO Move me into the UnitsNet library. This will solve a pain point in generically enumerating units for a quantity.
        public static IReadOnlyList<object> GetUnits(QuantityType quantity)
        {
            // Ex: Find unit enum type UnitsNet.Units.LengthUnit from quantity enum name QuantityType.Length
            Type unitEnumType = UnitEnumTypes.First(t => t.FullName == $"UnitsNet.Units.{quantity}Unit");
            return Enum.GetValues(unitEnumType).Cast<object>().Skip(1).ToArray();
        }

I still prefer the switch implementation described earlier since reflection is slow and we have the tools to generate the code required.

Pull request

I'm happy to help, let me know what you need.

Summarized: create a fork of UnitsNet on github, clone that fork locally with git, checkout a branch add-getunits, add commits and push, go back to your fork on github and create a pull request from that branch.

See https://help.github.com/articles/creating-a-pull-request-from-a-fork/

If you agree with my proposal, I think it's a matter of:

  1. Edit GenerateUnitSystemDefaultSourceCode.ps1 to add the method GetUnits() as described above, it generates UnitSystem.Default.g.cs partial class.
  2. Run generate-code.bat
  3. Update WPF sample to use UnitSystem.GetUnits() instead (if it references the nuget, then you have to wait until a new nuget is released)

P.S:
I just realized the switch cases can be simplified by reusing LengthUnit[] Units property of Length and similar for other quantities. No need to write unit enumeration code twice. However, I did notice we don't filter out Undefined values there, but we probably should. I'll create a separate issue for that.

@angularsen
Copy link
Owner

#437

@chrisnbray
Copy link
Author

Hey, it's your library! I am just grateful to have the use of the fruits of your labour...

@angularsen
Copy link
Owner

Hi, do you have any plans to pursue this?
If I read correctly, what remains is to add a way to dynamically query the units of a quantity - something like Array unitEnumValues = UnitAbbreviationsCache.Default.GetUnits(QuantityType);. It would remove the need for reflection in your initial example

Type unitEnumType = UnitEnumTypes.First(t => t.FullName == $"UnitsNet.Units.{quantity}Unit");
return Enum.GetValues(unitEnumType).Length > 2;

UnitSystem is split into new types in v4 branch, UnitAbbreviationsCache is relevant for this change:

private static readonly Tuple<string, Type, int, string[]>[] GeneratedLocalizations

Let me know if you want to proceed with a pull request on this or if we should close the issue.

@chrisnbray
Copy link
Author

chrisnbray commented Oct 15, 2018 via email

@angularsen
Copy link
Owner

Ouch, I'm terribly sorry to hear that :-/ I hope your partner recovers soon!
I'm happy to keep this issue open and check in on you at a later time, there is absolutely no rush. Take your time.

If there is anything I can help you with to get you started, let me know.

We do have a wiki that explains steps for adding new units, but other than that it should be fairly normal C# code. The only special thing perhaps is that we use a lot of code generation with PowerShell scripts. For this change you should not have to worry about it, but you run generate-code.bat to regenerate whenever you add new units in our .json definition files or want to make a change in the generated code.

GeneratedCode folders have generated code and CustomCode have code where we enrich the generated types, using partial classes. This can be adding new methods to Length type for instance, such as FromFeetInches().

@angularsen
Copy link
Owner

I am closing this issue due to inactivity. Please reopen when there is renewed interest. Also please see the PR #576 that improves working dynamically with quantities and things like converting and enumerating units.

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

2 participants