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

VolumeFlow conversion for UkGallonsPerHour is incorrect. #1389

Open
mswpearson opened this issue May 17, 2024 · 6 comments
Open

VolumeFlow conversion for UkGallonsPerHour is incorrect. #1389

mswpearson opened this issue May 17, 2024 · 6 comments
Labels

Comments

@mswpearson
Copy link

Describe the bug
The defined VolumeFlow conversion value for UkGallonsPerHour is incorrect by a significant amount.

To Reproduce
N/A

Expected behavior
The conversion value is configured as 791887.667, but after a google search and some manual testing I found the conversion value should be more like 791889.294.

This is a significant difference of around 2.5 units.

Screenshots
Google suggestion:
image

Failing unit test:
image

Manual generation of the real conversion value:
image

Configured value in VolumeFlow.json
image

Additional context
I was writing unit tests to ensure the right conversions were taking place in my application.

A test for the conversion between L/s to gal (imp.)/h was failing due to the values being not quite right, even to a small degree of accuracy.

This prompted me to find the root cause of the issue which appears to be this inaccurate configured conversion value.

Funnily enough, in the original commit for this quantity the conversion number and the value used in the tests is different! Maybe I'm misunderstanding that test value though. Original commit: 4eb94e9

@mswpearson mswpearson added the bug label May 17, 2024
@lipchev
Copy link
Collaborator

lipchev commented May 17, 2024

I've also noticed some issues while doing one of my PRs targeting v6. In my change-set I've tried to resolve them by unifying the conversions using the same constant:

VolumeFlow.json:

 {
   "SingularName": "CubicFootPerSecond",
   "PluralName": "CubicFeetPerSecond",
   "FromUnitToBaseFunc": "{x} * 0.028316846592",
   "FromBaseToUnitFunc": "{x} / 0.028316846592",
   "Localization": [
     {
       "Culture": "en-US",
       "Abbreviations": [ "ft³/s" ]
     }
   ]
 },
 {
   "SingularName": "CubicFootPerMinute",
   "PluralName": "CubicFeetPerMinute",
   "FromUnitToBaseFunc": "{x} * 0.028316846592 / 60",
   "FromBaseToUnitFunc": "{x} / (0.028316846592 / 60)",
   "Localization": [
     {
       "Culture": "en-US",
       "Abbreviations": [ "ft³/min", "CFM" ]
     }
   ]
 },
 {
   "SingularName": "CubicFootPerHour",
   "PluralName": "CubicFeetPerHour",
   "FromUnitToBaseFunc": "{x} * 0.028316846592 / 3600",
   "FromBaseToUnitFunc": "{x} / (0.028316846592 / 3600)",
   "Localization": [
     {
       "Culture": "en-US",
       "Abbreviations": [ "ft³/h", "cf/hr" ]
     }
   ]
 },

StandardVolumeFlow.json:

  {
    "SingularName": "StandardCubicFootPerSecond",
    "PluralName": "StandardCubicFeetPerSecond",
    "FromUnitToBaseFunc": "{x} * 0.028316846592",
    "FromBaseToUnitFunc": "{x} / 0.028316846592",
    "Localization": [
      {
        "Culture": "en-US",
        "Abbreviations": [ "Sft³/s" ]
      }
    ]
  },
  {
    "SingularName": "StandardCubicFootPerMinute",
    "PluralName": "StandardCubicFeetPerMinute",
    "FromUnitToBaseFunc": "{x} * 0.028316846592 / 60",
    "FromBaseToUnitFunc": "{x} / (0.028316846592 / 60)",
    "Localization": [
      {
        "Culture": "en-US",
        "Abbreviations": [ "scfm" ]
      }
    ]
  },
  {
    "SingularName": "StandardCubicFootPerHour",
    "PluralName": "StandardCubicFeetPerHour",
    "FromUnitToBaseFunc": "{x} * 0.028316846592 / 3600",
    "FromBaseToUnitFunc": "{x} / (0.028316846592 / 3600)",
    "Localization": [
      {
        "Culture": "en-US",
        "Abbreviations": [ "scfh" ]
      }
    ]
  }

Perhaps we could do the same for the Uk stuff as well?

@mswpearson
Copy link
Author

I like this approach a lot, keeps it consistent and accurate.

Do you have a PR in for this yet? If so would you be happy to make the uk fixes at the same time?

@lipchev
Copy link
Collaborator

lipchev commented May 21, 2024

Well, I was going to make one.. However there are a quite a few matches when we search for Hour so I was thinking of making one "Unifying the conversion format for all time based units".
@angularsen Do you think we should make this for v5 or v6?

@angularsen
Copy link
Owner

We can do both v5 and v6 if you are up for it 👍

@lipchev
Copy link
Collaborator

lipchev commented May 24, 2024

I've so far prepared the "correct" json files for Volume, VolumeFlow, StandardVolumeFlow, Mass, ForceChangeRate, AreaMomentOfInertia and Molarity. And I don't think I've even reached half of it yet..
So how do you want me to group the PRs? One for all or grouped somehow (like "Length", "Volume", "VolumeFlow", "StandardVolumeFlow").

I haven't had to change any of the unit tests so far, so given how I've used comment-links on each of the "basic conversion factors" (e.g. Pounds, Acres, Barrels etc.) it should be a pretty straightforward review.:

 {
   "SingularName": "BoardFoot",
   "PluralName": "BoardFeet",
   "XmlDocSummary": "The board foot or board-foot is a unit of measurement for the volume of lumber in the United States and Canada. It equals the volume of a board that is one-foot (305 mm) in length, one-foot (305 mm) in width, and one-inch (25.4 mm) in thickness.",
   "XmlDocRemarks": "https://en.wikipedia.org/wiki/Board_foot",
   "FromUnitToBaseFunc": "{x} * (0.028316846592 / 12)",
   "FromBaseToUnitFunc": "{x} / (0.028316846592 / 12)",
   "Localization": [
     {
       "Culture": "en-US",
       "Abbreviations": [ "bf", "board foot", "board feet" ]
     },
     {
       "Culture": "fr-CA",
       "Abbreviations": [ "pmp", "pied-planche", "pied de planche" ]
     }
   ]
 }

@angularsen
Copy link
Owner

I'm fine either way, no need for any particular grouping for review purposes.
But, since the change will be fairly similar across the board, then larger PRs may speed things up.
However we can merge in in smaller pieces if it makes it easier for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants