#6 add volume module #9

katjam · 2018-10-04T08:13:30Z

No description provided.

katjam · 2018-10-04T08:17:47Z

As discussed a WIP to make sure I'm headed in the right direction before pushing on with the rest of the implementation.

ianmackenzie · 2018-10-04T13:16:36Z

Looking good! As promised, a few small comments:

My initial thought was to leave out Cubed and cubed/cbrt and just have CubicMeters be its own units type with no particular relation to any other one (since I can't think of any purpose of cubed units other than volume - squared units are more generally useful for doing things like hypotenuse or root-mean-square calculations on arbitrary units). But it fits in well and is consistent with Squared/Area, so I think probably leave it as is.
To preview the documentation, you can use the excellent elm-doc-preview tool - generate the docs by running elm make --docs=docs.json in the root of your elm-units checkout, then upload docs.json and README.md to the doc preview tool. (Actually, if you're not editing the README, you might only need to upload docs.json.)
I think I might leave out cubicMillimeters and cubicCentimeters - I don't get the sense they're widely used, and a cubic centimeter is equivalent to a milliliter anyways.
If you could add a few relevant tests to tests/Tests.elm (a volumes test using equalPairs, and/or a "Volume" section in conversionsToQuantityAndBack), that would be awesome, but I'm also happy to do that myself. You'll need to install elm-test and run it from the root of the repository.

Oh, and go ahead and add yourself to the AUTHORS file =) You don't need to include your e-mail address if you'd prefer not to (I think GitHub username is fine).

katjam · 2018-10-04T13:39:16Z

I'm happy to do all of that and write the tests - I know what you mean about Cubed cubed cbrt- I wasn't sure either but it felt consistent. I'm out this evening but will be able to work on tomorrow morning.

harrysarson

I am do not have any authority so please feel free to ignore, but I noticed a potential problem with the cbrt function.

harrysarson · 2018-10-04T14:45:00Z

src/Quantity.elm

+-}
+cbrt : Quantity Float (Cubed units) -> Quantity Float units
+cbrt (Quantity value) =
+    Quantity (value ^ (1 /3))


For negative values this will give NaN. (i.e. in elm (-8) ^ (1/3) is NaN.)

This may be the desired behaviour but at the very least this should be documented as people may expect this to work for them.

Excellent point @harrysarson, great catch! Given that we don't have direct access to Math.cbrt from JavaScript, I think the implementation should probably be updated to

cbrt (Quantity value) = if value >= 0 then Quantity (value ^ (1 / 3)) else Quantity -((-value) ^ (1 / 3))

Does that make sense?

@harrysarson Thanks!

…o 6-add-volume-module

katjam · 2018-10-04T16:32:01Z

@ianmackenzie Do you want me to close this PR and open tomorrow when I think it's ready? Or are you happy for me to leave open and let you know?

ianmackenzie · 2018-10-04T17:29:19Z

No, I would say leave it open! Just keep adding commits to your branch, and add comments here when you have questions/want feedback/think things might be ready to merge etc. (Unless that's more annoying on your end? I don't want to add extra work...)

That's how I've done it with lots of other PRs and I think it's worked really well. I really see PRs as a sort of collaborative workspace where code can be added, changed, discussed by everyone and eventually (when everyone agrees) merged back into master. See ianmackenzie/elm-geometry#58, for example - after that one was opened we spent a month trying out different approaches, discussing different corner cases, adding some new features etc. before eventually merging something that was quite different from how it started. Frankly, I think it would work well to start a PR with no commits at all and then just do all work there...but I don't know if GitHub supports that!

katjam · 2018-10-04T21:12:18Z

Works for me. Sometimes on internal projects it's hard to tell when the person is ready to ask for review if the PR remains open and can end up in wasted time flagging up stuff that is in progress. But collaboration and continuous review sounds good to me for this. Would be nice if GitHub had a flag to say in progress Vs think this is ready for merge... Though just talking works too.

…

On Thu, 4 Oct 2018, 18:29 Ian Mackenzie, ***@***.***> wrote: No, I would say leave it open! Just keep adding commits to your branch, and add comments here when you have questions/want feedback/think things might be ready to merge etc. (Unless that's more annoying on your end? I don't want to add extra work...) That's how I've done it with lots of other PRs and I think it's worked really well. I really see PRs as a sort of collaborative workspace where code can be added, changed, discussed by everyone and eventually (when everyone agrees) merged back into master. See ianmackenzie/elm-geometry#58 <ianmackenzie/elm-geometry#58>, for example - after that one was opened we spent a *month* trying out different approaches, discussing different corner cases, adding some new features etc. before eventually merging something that was quite different from how it started. Frankly, I think it would work well to start a PR with *no commits at all* and then just do *all* work there...but I don't know if GitHub supports that! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0j9F3YkrBv4_iL0eJFOayCf0jNQR2lks5uhkVwgaJpZM4XHlbY> .

katjam · 2018-10-05T11:38:01Z

src/Volume.elm

+-}
+liters : Float -> Volume
+liters numLiters =
+    cubicMeters (1.0e-6 * numLiters)


Has been corrected to 0.001

katjam · 2018-10-05T11:38:12Z

src/Volume.elm

+-}
+inLiters : Volume -> Float
+inLiters volume =
+    1.0e6 * inLiters volume


Has been corrected to 1000

katjam · 2018-10-05T17:59:47Z

src/Volume.elm

+-}
+usLiquidGallons : Float -> Volume
+usLiquidGallons numUsLiquidGallons =
+    cubicMeters ( numUsLiquidGallons / 264.17220000000003 )


This might be a bit too extreme - but no harm done. @ianmackenzie Are you using any particular definitive doc to get the conversion factors?

Typically I've just been scouring Wikipedia - for U.S. liquid gallons is there a reason not to use numUsLiquidGallons * 0.003785411784? https://en.wikipedia.org/wiki/Gallon#US_liquid_gallon

I'm open to using a single definitive source if there's a good one...

Going in that direction I got a source to go up to 0.003785409668390542 And I thought it was a bit too much. Using 264.1722 seemed acceptable by many sources.
That's why I asked because I guess it would be good to try and reference one scientific body's docs rather than different ones.

My Current implementation returns:

Volume.inCubicMeters (Volume.usLiquidGallons 1) >>0.0037854096683905417 : Float

Yeah, I guess it would good to find one reference - I'll try to evaluate some options. If you have any candidates, let me know!

Have had a little look around - nothing jumps out. Countries and disciplines seem to publish their own standards. I haven't done any comparing though to check for similarities. There's not an obvious international body.

I'm having trouble with the different Gallons - opinions seem to vary after the 3 or 4 significant digit. I'm starting to think we'd be better off only going to 4 significant digits for these calculations because of that... but have submitted with Quart support & a few tests!

Would it be a bad idea to grab the conversion constants I'm using in multiple places and stick them in functions for reuse? I don't think you've been doing that - so am trying to keep a consistent style.

I should be able to get the Pints and fl oz in today - so this hopefully will be ready to merge.

Happy to alter my non-metric liquid conversion factors to better ones if you manage to find any.

harrysarson

I have some nits

src/Quantity.elm

harrysarson · 2018-10-06T14:23:53Z

src/Volume.elm

--@docs usFluidOunces, imperialFluidOunces
+@docs usLiquidQuarts, inUsLiquidQuarts, usDryQuarts, inUsDryQuarts, imperialQuarts, inImperialQuarts
+@docs --@docs usLiquidPints, usDryPints, imperialPints
+@docs --@docs usFluidOunces, imperialFluidOunces


@docs appears twice here, once before and once after the comment.

katjam · 2018-10-06T15:48:54Z

src/Volume.elm

+@docs usLiquidGallons, inUsLiquidGallons, usDryGallons, inUsDryGallons, imperialGallons, inImperialGallons
+@docs usLiquidQuarts, inUsLiquidQuarts, usDryQuarts, inUsDryQuarts, imperialQuarts, inImperialQuarts
+--@docs usLiquidPints, usDryPints, imperialPints
+--@docs usFluidOunces, imperialFluidOunces


elm-format adds the extra @docs leader every time - have left these here for ref but will be sorted before it's ready to merge.

katjam · 2018-10-06T16:48:59Z

@ianmackenzie Some of the conversion factors might need tweaking (and possibly moved out into constant functions) as discussed.

Might also be some more equalPairs we should do - but I'm happy for this to go in as it is if you are.

ianmackenzie · 2018-10-06T17:51:26Z

I think it would probably make sense to pull out some of the gallon conversion factors into (non-exposed) Float constants. I hadn't been doing that in other modules since in most cases the constants only showed up in two places right beside each other, but here those factors show up in a bunch of different places far apart in the file.

The alternative would be to define (for example) usLiquidQuarts in terms of usLiquidGallons instead of converting directly to cubic meters:

usLiquidQuarts : Float -> Volume
usLiquidQuarts numUsLiquidQuarts =
    usLiquidGallons (numUsLiquidQuarts / 4)

inUsLiquidQuarts : Volume -> Float
inUsLiquidQuarts volume =
    4 * inUsLiquidGallons volume

Perhaps slightly less efficient (one extra function call), but I suspect that if you're working with "non-technical" units like quarts then you're less likely to be doing high-performance numerical code anyways. And it's easy enough to optimize later if that does become an issue. Thoughts?

I think once we make a decision there this is probably ready to merge - I think it makes sense to open a new issue for switching to a single standard reference for conversion factors.

ianmackenzie · 2018-10-06T19:47:50Z

Just added #10 to figure out a more rigorous way of determining conversion factors - would love to get your thoughts!

katjam · 2018-10-06T23:33:59Z

I thought about composing the functions together at first but I think I prefer keeping all the functions as direct to and from Cubic Meters. Makes them more independent (not sure on the technical term) and feels cleaner. So can we go with the non-exposed constants? (Until I change my mind - when I find it was a bad decision...)

ianmackenzie · 2018-10-07T01:49:35Z

Yup, I'm totally happy with the constants approach! We can also reassess later when we make a decision about what reference(s) to use for the conversion factors - it could be useful for the structure of the code to match up closely with how things are listed in whatever document we reference.

katjam · 2018-10-07T11:40:16Z

OK I think this is ready now. I stuck some equal pairs tests in so we remember to verify relationships again when we settle on some conversion factors.

I'm open you renaming the constants before you merge. They seem a bit verbose but others that I considered didn't seem as clear.

usDryGallonsInCubicMeter
cubicMeterToUsDryGallonFactor
usDryGallonsPerCubicMeterper seems to be used in conjunction with rate mostly, so although this is the most concise - it may not be correct terminology.

katjam · 2018-10-07T16:00:33Z

Looks good.

ianmackenzie · 2018-10-07T16:06:42Z

Just renamed the constants to the usDryGallonsPerCubicMeter style - I think conversion factors probably will end up being defined as 'rates' in a lot of different situations. (Perhaps there's a better word than 'rate' that better encompasses both 'rate of change' and 'conversion ratio', but I'm not sure what that is.)

Also took the liberty of rewording the docs to say things like "Construct a volume from a number of U.S. dry pints" instead of "Construct a volume from a number of usDryPints", and made a couple of other minor formatting tweaks. I think it is indeed ready to merge!

katjam and others added 3 commits October 3, 2018 19:06

Provide initial volume functions

e22c518

Add cubed and cbrt support to Quantity

b0986d7

Merge branch 'master' into 6-add-volume-module

4e12a92

harrysarson suggested changes Oct 4, 2018

View reviewed changes

katjam added 2 commits October 4, 2018 17:20

Remove unneccessary cm and mm function. Correct cbtr.

41d3a1c

Merge branch '6-add-volume-module' of github.com:katjam/elm-units int…

6e908c9

…o 6-add-volume-module

ianmackenzie mentioned this pull request Oct 4, 2018

Add more quantity types #6

Open

15 tasks

Tidier docs and remove deciliter stubs (no immediate need).

eed25b3

katjam commented Oct 5, 2018

View reviewed changes

Gallon conversions and correct Liter/Milliliter

8edc9f4

katjam commented Oct 5, 2018

View reviewed changes

katjam added 3 commits October 5, 2018 19:43

Volume fuzz tests and responsibility

ccf08da

Convert to Quarts

89118f8

Remove stray @doc added by elm-format

1c8b749

harrysarson approved these changes Oct 6, 2018

View reviewed changes

katjam commented Oct 6, 2018

View reviewed changes

katjam added 2 commits October 6, 2018 17:20

Add pints to Volume

69c4b99

Add fluid ounces to Volume

a041b4e

Include conversion factors as helper constants

348f813

ianmackenzie added 5 commits October 7, 2018 11:37

Tweak doc formatting

a85a6aa

Rename volume conversion constants

8a26370

Reword Volume doc comments

d560643

Merge remote-tracking branch 'upstream/master' into 6-add-volume-module

8d6c6ba

Tweak documentation wording

59b5d5c

ianmackenzie merged commit 2950530 into ianmackenzie:master Oct 7, 2018

katjam deleted the 6-add-volume-module branch October 7, 2018 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#6 add volume module #9

#6 add volume module #9

katjam commented Oct 4, 2018

katjam commented Oct 4, 2018

ianmackenzie commented Oct 4, 2018

katjam commented Oct 4, 2018

harrysarson left a comment

harrysarson Oct 4, 2018

ianmackenzie Oct 4, 2018

katjam Oct 4, 2018

katjam commented Oct 4, 2018

ianmackenzie commented Oct 4, 2018

katjam commented Oct 4, 2018 via email

katjam Oct 5, 2018

katjam Oct 5, 2018

katjam Oct 5, 2018

ianmackenzie Oct 5, 2018

katjam Oct 5, 2018

ianmackenzie Oct 5, 2018

katjam Oct 6, 2018

harrysarson left a comment

harrysarson Oct 6, 2018

katjam Oct 6, 2018

katjam commented Oct 6, 2018

ianmackenzie commented Oct 6, 2018

ianmackenzie commented Oct 6, 2018

katjam commented Oct 6, 2018

ianmackenzie commented Oct 7, 2018

katjam commented Oct 7, 2018

katjam commented Oct 7, 2018

ianmackenzie commented Oct 7, 2018

#6 add volume module #9

#6 add volume module #9

Conversation

katjam commented Oct 4, 2018

katjam commented Oct 4, 2018

ianmackenzie commented Oct 4, 2018

katjam commented Oct 4, 2018

harrysarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katjam commented Oct 4, 2018

ianmackenzie commented Oct 4, 2018

katjam commented Oct 4, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harrysarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katjam commented Oct 6, 2018

ianmackenzie commented Oct 6, 2018

ianmackenzie commented Oct 6, 2018

katjam commented Oct 6, 2018

ianmackenzie commented Oct 7, 2018

katjam commented Oct 7, 2018

katjam commented Oct 7, 2018

ianmackenzie commented Oct 7, 2018