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

Rename SpecificFuelConsumption.GramPerKiloNewtonSecond to GramPerKilonewtonSecond #1491

Merged
merged 4 commits into from
Jan 19, 2025

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented Jan 3, 2025

Rename SpecificFuelConsumption.GramPerKiloNewtonSecond to GramPerKilonewtonSecond (part of #1200)

Comment on lines 1337 to 1339
"PoundMassPerPoundForceHour": 4
"PoundMassPerPoundForceHour": 4,
"GramPerKilonewtonSecond": 11,
"KilogramPerKilonewtonSecond": 6
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here:

This maybe a little hack-y, but I think that if we changed these by hand (to the old values) - I think they would stick.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I suggest we do the same here, match the old values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 730eab1

Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion, other than that ready to merge

Comment on lines 1337 to 1339
"PoundMassPerPoundForceHour": 4
"PoundMassPerPoundForceHour": 4,
"GramPerKilonewtonSecond": 11,
"KilogramPerKilonewtonSecond": 6
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I suggest we do the same here, match the old values.

@lipchev
Copy link
Collaborator Author

lipchev commented Jan 3, 2025

Hmm, I just realized that in my other project I actually have the SI coverage tests [Skipped] for this one..

As you've correctly observed in #1473 the BaseUnits for the GramPerKilonewtonSecond also simplify to Meter, Second

{
  "SingularName": "GramPerKilonewtonSecond",
  "PluralName": "GramsPerKilonewtonSecond",
  "BaseUnits": {
    "L": "Meter",
    "T": "Second"
  },
  "FromUnitToBaseFunc": "{x}",
  "FromBaseToUnitFunc": "{x}",
  "Prefixes": [ "Kilo" ],
  "Localization": [
    {
      "Culture": "en-US",
      "Abbreviations": [ "g/(kN�s)" ],
      "AbbreviationsForPrefixes": { "Kilo": [ "kg/(kN�s)" ] }
    }
  ]
}

However this showcases another limitation of using the BaseUnits to determine the proper SI unit for quantities having a hidden dimension.

The problem is even worse, when this is the Mass dimension, with it's goddamn Kilo prefix.. Even if we removed the BaseUnits from the GramPerKilonewtonSecond and introduced a brand new unit

    {
      "SingularName": "GramPerNewtonSecond",
      "PluralName": "GramsPerNewtonSecond",
      "BaseUnits": {
        "L": "Meter",
        "T": "Second"
      },
      "FromUnitToBaseFunc": "{x} * 1000",
      "FromBaseToUnitFunc": "{x} / 1000",
      "Prefixes": [ "Kilo" ],
      "Localization": [
        {
          "Culture": "en-US",
          "Abbreviations": [ "g/(N�s)" ],
          "AbbreviationsForPrefixes": { "Kilo": [ "kg/(N�s)" ] }
        }
      ]
    }

This still wouldn't give us the proper SI unit, as the Kilo prefix would scale down the Length dimension, turning the BaseUnits to Millimeter, Second.

The only way this can be made right is to have the KilogramsPerNewtonSecond as it's own non-prefixed unit, with the rest of the units being scaled accordingly (this must be why I had the Microsecond in it's BaseUnits #1473 🤷 ).

PS A little more of this and the doctor's diagnosis is going to be brain-fucked.. 🧠

@lipchev
Copy link
Collaborator Author

lipchev commented Jan 3, 2025

The problem is even worse, when this is the Mass dimension, with it's goddamn Kilo prefix.. Even if we removed the BaseUnits from the GramPerKilonewtonSecond and introduced a brand new unit

    {
      "SingularName": "GramPerNewtonSecond",
      "PluralName": "GramsPerNewtonSecond",
      "BaseUnits": {
        "L": "Meter",
        "T": "Second"
      },
      "FromUnitToBaseFunc": "{x} * 1000",
      "FromBaseToUnitFunc": "{x} / 1000",
      "Prefixes": [ "Kilo" ],
      "Localization": [
        {
          "Culture": "en-US",
          "Abbreviations": [ "g/(N�s)" ],
          "AbbreviationsForPrefixes": { "Kilo": [ "kg/(N�s)" ] }
        }
      ]
    }

This still wouldn't give us the proper SI unit, as the Kilo prefix would scale down the Length dimension, turning the BaseUnits to Millimeter, Second.

The only way this can be made right is to have the KilogramsPerNewtonSecond as it's own non-prefixed unit, with the rest of the units being scaled accordingly (this must be why I had the Microsecond in it's BaseUnits #1473 🤷 ).

PS A little more of this and the doctor's diagnosis is going to be brain-fucked.. 🧠

Oh I think I made a mistake here- we won't actually need KilogramsPerNewtonSecond to be an independent unit, the BaseUnits of Millimeter, Second would actually scale up to the Meter, Second for the KilogramPerNewtonSecond.

@lipchev
Copy link
Collaborator Author

lipchev commented Jan 3, 2025

The problem is even worse, when this is the Mass dimension, with it's goddamn Kilo prefix.. Even if we removed the BaseUnits from the GramPerKilonewtonSecond and introduced a brand new unit

    {
      "SingularName": "GramPerNewtonSecond",
      "PluralName": "GramsPerNewtonSecond",
      "BaseUnits": {
        "L": "Meter",
        "T": "Second"
      },
      "FromUnitToBaseFunc": "{x} * 1000",
      "FromBaseToUnitFunc": "{x} / 1000",
      "Prefixes": [ "Kilo" ],
      "Localization": [
        {
          "Culture": "en-US",
          "Abbreviations": [ "g/(N�s)" ],
          "AbbreviationsForPrefixes": { "Kilo": [ "kg/(N�s)" ] }
        }
      ]
    }

This still wouldn't give us the proper SI unit, as the Kilo prefix would scale down the Length dimension, turning the BaseUnits to Millimeter, Second.
The only way this can be made right is to have the KilogramsPerNewtonSecond as it's own non-prefixed unit, with the rest of the units being scaled accordingly (this must be why I had the Microsecond in it's BaseUnits #1473 🤷 ).
PS A little more of this and the doctor's diagnosis is going to be brain-fucked.. 🧠

Oh I think I made a mistake here- we won't actually need KilogramsPerNewtonSecond to be an independent unit, the BaseUnits of Millimeter, Second would actually scale up to the Meter, Second for the KilogramPerNewtonSecond.

Ok, I think I almost got it figured out- for the GramPerNewtonSecond we could have the BaseUnits defined as either Meter, Millisecond or Kilometer, Second, however the UnitPrefixBuilder isn't impressed by my attempts to trick it into giving me a non-prefixed unit.. In the first case it was able to derive the Millimeter, Millisecond, and the second one returning Undefined..

This is because the BaseUnitPrefixes aren't storing the non-prefixed units (corresponding to a prefix-exponent of 0). I don't know if there are any other cases that aren't being de-prefixed like that, but I'm going try to fix the BaseUnitPrefixes and see what comes out..

@lipchev
Copy link
Collaborator Author

lipchev commented Jan 4, 2025

Ok, I think I almost got it figured out- for the GramPerNewtonSecond we could have the BaseUnits defined as either Meter, Millisecond or Kilometer, Second, however the UnitPrefixBuilder isn't impressed by my attempts to trick it into giving me a non-prefixed unit.. In the first case it was able to derive the Millimeter, Millisecond, and the second one returning Undefined..

This is because the BaseUnitPrefixes aren't storing the non-prefixed units (corresponding to a prefix-exponent of 0). I don't know if there are any other cases that aren't being de-prefixed like that, but I'm going try to fix the BaseUnitPrefixes and see what comes out..

Good, it seems this wasn't all in vain - #1492 turned up with quite a few new options for the BaseUnits. I also tried the two cases that were failing before - and they now both produce Meter, Second.

Anyway, this PR was only about renaming the unit, so it should be good to go. For the rest of it, I'll open another one later..

@angularsen angularsen merged commit 4431e85 into angularsen:release/v6 Jan 19, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants