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

Support multiple abbreviations for kilo, mega and other prefixes of units #344

Closed
angularsen opened this issue Dec 16, 2017 · 8 comments · Fixed by #658
Closed

Support multiple abbreviations for kilo, mega and other prefixes of units #344

angularsen opened this issue Dec 16, 2017 · 8 comments · Fixed by #658

Comments

@angularsen
Copy link
Owner

Problem

Currently, it is not possible to support parsing both Area.Parse("1 km²") and Area.Parse("1 km^2") due to a limitation in the JSON structure for prefix units (kilo, mega etc.) and how we parse it in powershell script.

Background

We support defining multiple abbreviations for a unit in JSON on the Abbreviations property, but many units add prefixes such as kilo, mega etc to generate those extra units by simply adding k and M prefix to the unit abbreviation. However, this design currently has two limitations:

  1. Only the first abbreviation in Abbreviations is used when creating abbreviations for prefix units. Ex: Area may have "Abbreviations": [ "m²", "m^2" ] and "Prefixes": [ "Kilo" ], but it only generates km² for the square kilometer unit.

  2. If AbbreviationsWithPrefixes is defined, then this should be used instead of adding k and M prefixes. This is a list of abbreviations fully typed out and each position in the array matches the array Prefixes. This is useful for languages such as Russian where a different character should be used or where it's not as simple as prefixing a string. However, currently this is an array of strings and thus only supports 1 abbreviation per prefix. I propose to extend this to support an array of a mix of strings and arrays of strings.

Proposal

Support defining multiple strings per prefix unit in JSON, but keep it backwards compatible so we can either define an array of strings (as we already do) or an array of arrays of strings.

The following (silly) example would create the following abbreviations: m², m^2, km², km^2, Mm²
I think in most cases it would be either an array of string OR an array of arrays of strings - and not a mix as in this example, but implementing it this way makes it flexible and should be a matter of checking each item in AbbreviationsWithPrefixes whether it is a string or an array.

"Localization": [
          {
            "Culture": "en-US",
            "Abbreviations": [ "m²", "m^2" ],
            "Prefixes": [ "Kilo", "Mega" ],
            "AbbreviationsWithPrefixes": [ ["km²", "km^2"], "Mm²" ] // contrived example
          }
  ]
@angularsen
Copy link
Owner Author

Closing this due to inactivity.

@angularsen
Copy link
Owner Author

Reopening this due to blocking #646 .

@angularsen angularsen reopened this Apr 14, 2019
@lipchev
Copy link
Collaborator

lipchev commented Apr 14, 2019

Here is what I tried with (first time PS):

Localization=$unit.Localization | % { 
   					foreach ($abbrSyno in $_.Abbreviations) {
   						$abbrev = $prefixAbbreviation + $abbrSyno
   						if ($_.AbbreviationsWithPrefixes) {
   							if($_.AbbreviationsWithPrefixes[$prefixIndex] -isnot [System.String]){
   								foreach($synoWithPrefix in $_.AbbreviationsWithPrefixes[$prefixIndex]){		
   									New-Object PsObject -Property @{
   										Culture=$_.Culture
   										Abbreviations= $synoWithPrefix
   									}
   								}
   								continue
   							}
   							else{
   								$abbrev = $_.AbbreviationsWithPrefixes[$prefixIndex]
   							}
   						}
   						New-Object PsObject -Property @{
   							Culture=$_.Culture
   							Abbreviations= $abbrev
   						}
   					}
   				}

It seems to work for

{
     "Prefixes": [ "Pico", "Nano", "Micro", "Milli", "Centi", "Deci" ],
     "Localization": [
       {
         "Culture": "en-US",
         "Abbreviations": [ "mol/L", "M"]
       }
     ]
   }

However when I tried to alter the mass with ..

"Localization": [
        {
          "Culture": "en-US",
          "Abbreviations": [ "g" ]
        },
        {
          "Culture": "ru-RU",
          "Abbreviations": [ "г" ],
          "AbbreviationsWithPrefixes": [ "нг", "мкг", "мг", "сг", "дг", "даг", "гг", [ "кг", "кг." ] ]
        }

.. I got a list of strings and no arrays (the AbbrWithPrefix is array condition is never reached).
Any tips?

@angularsen
Copy link
Owner Author

I don't immediately see the error her, and I kind of struggle to read the above code snippet and the flow.
Do you have a branch I can checkout and look at the code locally with an IDE?

@angularsen
Copy link
Owner Author

Just create a pull request and we can discuss there.

@lipchev
Copy link
Collaborator

lipchev commented Apr 15, 2019

Ok I figured it out- it works if you change the Localization.AbbreviationsWithPrefixes to [object[]]. Was that your intention?
Also found an issue(not related to the abbreviations) with (for example) Mass.Parse("1 кг", RussianCulture) - here the format provider gets lost..
Does it merit a separate issue?

@angularsen
Copy link
Owner Author

  1. object[] seems reasonable since we want it to be an array of items either string or string[] yes.
  2. I don't fully understand, I believe in your example you try to parse 1 kg in Russian. Are you saying that it fails to parse that? How does it fail?

@angularsen
Copy link
Owner Author

@lipchev A heads up, but yesterday I decided to rewrite the codegen scripts from PowerShell to C# in #656. Too many papercuts with the limited type system in PS over the years.
Depending on what PR is merged first, one of us will have a merge conflict to deal with.

angularsen pushed a commit that referenced this issue Apr 21, 2019
* Concentration Units (mixtures/solutions)

Added the following Concentration Units:
- MassConcentration: SI = kg/m3, typically mg/l
- VolumeConcentration : dimensionless, typically %
- MassFraction: SI = kg/kg, typically mg/kg

Modified the existing Molarity unit:
- Some operations that were originally based on the Density units now use the MassConcentration units instead (Note: despite the fact that they share the same measurement units- the Density is a distnct QuantyType from the MassConcentration)
- Removed all operators involving Molarity from the Density units

Defined some basic operations that were missing from the AmountOfSubstance/MolarMass/Mass units

Defined the triangular operations involving Mass/Molar/Volume concentrations (& the corresponding component's Density & MolarMass)

All unit tests included most were defined by actual chemists(which I AM NOT).
Note: one of the tests (QuantityIFormattableTests.VFormatEqualsValueToString)- was failing on my machine- it passes if I add CultureInfo.CurrentUICulture to the value.ToString() - as I presume was the intended behavior

* updated uppercase 'L' & BaseUnits in json

- updated liter abbreviations for g/l, g/dl, g/ml & kg/l to uppercase 'L'  (TODO Density?)
- added base units to all units in MassConcentration & Molarity (TODO Density?)

* BaseUnits, Obsolete methods & cosmetics

- corrected the BaseUnits for MassConcentration
- marked the invalid methods from Molarity/Density as Obsolete (were previously omitted)
- some cosmetic changes to the Unit Tests

* Removed Molarity.Molar as redundant (added abbreviation instead)

- MolesPerLiter: fixed the BaseUnits (default) to Deimeter/Mole
- Molar: removed in favor of using the alternative abbreviation 'M"
- MolarityTests - OneMilliMolarFromStringParsedCorrectly skipped while awaiting fix for #344

* Added a KnownQuantities class

- added a KnownQuantities class with a few constants that were used in multiple tests
- replaced the usages in MassConcentrationTests MolarityTests * VolumeConcentrationTests

* Testing with Theory + InlineData

- converted two of the MassConcentration tests to using Theory + InlineData

* Tests refactoring (Theory + InlineData)

- removed BaseUnits from GramPerDeciliter(not exact + overlap), kept it in GramPerLiter (as exact & non-overlapping), also kept it for GramPerMilliliter(exact + overlapping) because I thought it would be useful to have at least one such case for future testing
- moved the Mass/MolarMass operator to the Mass class (removing the MolarMass.extra)
- all tests refactored using Theory + Inline Data
- moved one or two tests to the appropriate .Test file
- removed a few redundant tests

* Make single line act-statements in tests
angularsen pushed a commit that referenced this issue Apr 22, 2019
* Concentration Units (mixtures/solutions)

Added the following Concentration Units:
- MassConcentration: SI = kg/m3, typically mg/l
- VolumeConcentration : dimensionless, typically %
- MassFraction: SI = kg/kg, typically mg/kg

Modified the existing Molarity unit:
- Some operations that were originally based on the Density units now use the MassConcentration units instead (Note: despite the fact that they share the same measurement units- the Density is a distnct QuantyType from the MassConcentration)
- Removed all operators involving Molarity from the Density units

Defined some basic operations that were missing from the AmountOfSubstance/MolarMass/Mass units

Defined the triangular operations involving Mass/Molar/Volume concentrations (& the corresponding component's Density & MolarMass)

All unit tests included most were defined by actual chemists(which I AM NOT).
Note: one of the tests (QuantityIFormattableTests.VFormatEqualsValueToString)- was failing on my machine- it passes if I add CultureInfo.CurrentUICulture to the value.ToString() - as I presume was the intended behavior

* updated uppercase 'L' & BaseUnits in json

- updated liter abbreviations for g/l, g/dl, g/ml & kg/l to uppercase 'L'  (TODO Density?)
- added base units to all units in MassConcentration & Molarity (TODO Density?)

* BaseUnits, Obsolete methods & cosmetics

- corrected the BaseUnits for MassConcentration
- marked the invalid methods from Molarity/Density as Obsolete (were previously omitted)
- some cosmetic changes to the Unit Tests

* Removed Molarity.Molar as redundant (added abbreviation instead)

- MolesPerLiter: fixed the BaseUnits (default) to Deimeter/Mole
- Molar: removed in favor of using the alternative abbreviation 'M"
- MolarityTests - OneMilliMolarFromStringParsedCorrectly skipped while awaiting fix for #344

* Added a KnownQuantities class

- added a KnownQuantities class with a few constants that were used in multiple tests
- replaced the usages in MassConcentrationTests MolarityTests * VolumeConcentrationTests

* Testing with Theory + InlineData

- converted two of the MassConcentration tests to using Theory + InlineData

* Tests refactoring (Theory + InlineData)

- removed BaseUnits from GramPerDeciliter(not exact + overlap), kept it in GramPerLiter (as exact & non-overlapping), also kept it for GramPerMilliliter(exact + overlapping) because I thought it would be useful to have at least one such case for future testing
- moved the Mass/MolarMass operator to the Mass class (removing the MolarMass.extra)
- all tests refactored using Theory + Inline Data
- moved one or two tests to the appropriate .Test file
- removed a few redundant tests

* Make single line act-statements in tests

* Updated scripts to iterate over abbreviations

- Types.psm1: type of AbbreviationsWithPrefixes changed to object[]
- GenerateUnits.ps1: Add-PrefixUnits now iterates over all abbreviations (incl. with prefix)
- Duration: added the cyrilic equivalent to  "s": "sec" ("с", "сек")  to both the main and the prefixed abbrviations
- DurationTests: added a Theory with some of the main test cases for parsing seconds & milliseconds
- MolarityTests: removed the Skip for the awaiting test

* Update scripts WRC

- same fix applied to  the WRC project
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

Successfully merging a pull request may close this issue.

2 participants