Skip to content

Conversation

trb5016
Copy link
Contributor

@trb5016 trb5016 commented Jun 25, 2021

Added Volumetric Heat Capacity quantity and common units.

@trb5016
Copy link
Contributor Author

trb5016 commented Jun 25, 2021

Seeing all those extra changes to other files from the code gen was new to me - let me know if I messed something up.

Thanks!

@angularsen angularsen force-pushed the volumetric-heat-capacity branch from 00825ee to 0e08b67 Compare June 25, 2021 22:14
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.

I cleaned up the spam of changed files. It was a mistake in the codegen in the master branch.

Just a question below, other than that this looks good to me!

protected override double KilojoulesPerCubicMeterKelvinInOneJoulePerCubicMeterKelvin => 1E-3;
protected override double MegajoulesPerCubicMeterKelvinInOneJoulePerCubicMeterKelvin => 1E-6;

protected override double BtusPerCubicFootDegreeFahrenheitInOneJoulePerCubicMeterKelvin => 1.491066E-5;
Copy link
Owner

Choose a reason for hiding this comment

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

I've noticed earlier that there are BTU (th) and BTU (IT) units.
I don't know these units well, should both be included? Should we distinguish them in their unit names?

The one you defined seems to be IT.

British thermal unitIT per cubic foot degree Fahrenheit: 1.491066e-5
British thermal unitth per cubic foot degree Fahrenheit: 1.492065e-5

https://www.gordonengland.co.uk/conversion/volheatcap.htm

IT vs th:
https://www.translatorscafe.com/unit-converter/en-US/energy/32-33/Btu%20(IT)-Btu%20(th)/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for cleaning up those files.

I did indeed pick BTU(IT) and I think that is appropriate. Reasoning as follows:

  1. I've been in industry for 10 years (in the US) and in school for 7. Until I added this quantity (and used the same converter you found to check) I had never heard of these 'flavors' of BTUs. Based on memory of the numbers, I think I've always seen Btu(IT)

  2. https://www.gordonengland.co.uk/conversion/energy.htm This site indicates Btu(IT) is the 'default'.

  3. The existing Units.Net definition for Energy.BritishThermalUnit is a factor of 1055.05585262 from a Joule. This means it's a Btu(IT). Therefore, for the Units.Net Volumetric Heat Capacity BTU to be consistent with energy, it must also be Btu(IT)

Funny you should raise this question because I was debating making an issue for it separately because I went down a rabbit hole trying to figure this out. I ultimately decided against it because 1) it might be a breaking change, and 2) I think what we have now works for "practical" usage.

The extra confusing part is the default 'calorie' energy unit seems to be the (th) version, not the cal(IT) version (which apparently also exists. So in Units.Net (and I think "most" of the world) you have cal(th) and BTU(IT); but cal(IT) and Btu(th) also exist, as well as cal(mean) and Btu(mean).

The options for "fixing" this is to either:

Define calories/Btus explicitly with their "flavor" (i.e., IT, th, mean), or
Leave the current ones defined as they are, then add the extra "flavors" in we don't have defined.

The first is confusing, because I bet most people would have no idea what the appropriate unit is to choose (myself included) and it's a breaking change because you're renaming the unit.

The second is also confusing because now you have some cal & BTUs units that are explicitly "flavored" and others that aren't, and they're not consistent with each other. This is maybe okay? But doesn't seem worth adding unless someone explicitly needs it. I certainly don't because again, before last Friday - I had no idea this was a thing.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree with your reasoning. I believe the same argument has been made before, that IT is the widely used one and Th is the oddity. I also like that we are at least consistent within Units.NET.

@trb5016 trb5016 requested a review from angularsen June 29, 2021 13:09
@angularsen angularsen merged commit edc05e1 into angularsen:master Jun 29, 2021
@angularsen
Copy link
Owner

Thanks! Nuget is on the way out.

Release UnitsNet/4.97.0 · angularsen/UnitsNet

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 this pull request may close these issues.

2 participants