#6 add volume module

[katjam]

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

[ianmackenzie]

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]

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.

[katjam]

@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]

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 https://github.com/ianmackenzie/elm-geometry/pull/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]

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, notifications@github.com 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 https://github.com/ianmackenzie/elm-geometry/pull/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 https://github.com/ianmackenzie/elm-units/pull/9#issuecomment-427103745, or mute the thread https://github.com/notifications/unsubscribe-auth/AA0j9F3YkrBv4_iL0eJFOayCf0jNQR2lks5uhkVwgaJpZM4XHlbY .

[katjam]

@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]

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]

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

[katjam]

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]

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]

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]

Looks good.

[ianmackenzie]

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!