Add US Therm, EU Therm and UK Therm #287

DInozemtsev · 2017-09-14T13:37:54Z

No description provided.

eriove · 2017-09-14T18:33:55Z

Thanks for the pull request. I'm trying to find more information on thermal units. I've only succeeded in finding the British Thermal Unit (BTU). Do you have any references describing the ones you have added?

DInozemtsev · 2017-09-14T20:05:44Z

Hello there.

I have used this Wikipedia article for reference https://en.wikipedia.org/wiki/Therm.
Google Unit converter provides very close value on 1 US Therm to 1 Joule conversion. Check this out https://www.google.com/search?q=1+therm+in+joule

DInozemtsev · 2017-09-16T12:41:04Z

Sirs, is there something else that I could do to help you with this Pull Request? If so just let me know. :)

eriove

Sorry for the slow reply. I finally had time to do a proper review (see questions in code).

I also noticed that there are some questions regarding a mix up of ISO and IT numbers in the talk page on Wikipedia. It would be good if you could cross check the numbers once more since I don't know enough to determine what's correct.

eriove · 2017-09-17T17:29:01Z

UnitsNet.Tests/CustomCode/EnergyTests.cs

        protected override double ElectronVoltsInOneJoule => 6.241509343260179e18;

        protected override double ErgsInOneJoule => 10000000;

+        protected override double EuThermsInOneJoule => 9.4781339449889105832843629746176e-9;


I get this to 9.47816988e-9 when doing 1/105505600 in Google

Yeah, make sense, I'll change this value.

eriove · 2017-09-17T17:31:55Z

UnitsNet.Tests/GeneratedCode/EnergyTestsBase.g.cs

@@ -54,31 +54,43 @@ public abstract partial class EnergyTestsBase
    {
        protected abstract double BritishThermalUnitsInOneJoule { get; }
        protected abstract double CaloriesInOneJoule { get; }
+        protected abstract double DecaeuThermsInOneJoule { get; }


@anjdreas I would say that this should have a capital E since it is an abbreviation. What do you think? (Same with the others)

I agree, this doesn't look quite right. These are probably generated by powershell and the deca prefix in JSON. Deca + camelcase(EuTherms). This works for things like Decigram and Millinewtonmeters, but not for DecieuTherm. Not sure how we would fix this other than not using prefix and instead adding DecaEuTherm manually in JSON. Probably the easiest fix.

On second thought, DecaEuTherm is probably not consistent with our existing naming either. Maybe Decaeutherm, or Decaeurotherm as eurotherm seems to be a wording I find when googling this.
I might prefer Eurotherm and Decaeurotherm after chewing on it a bit.

@anjdreas Please also note that we also have DecaimperialThermsInOneJoule, DecausThermsInOneJoule. So to keep things simple at the moment, I would use ReSharper to rename this properties to DecaEuThermsInOneJoule , DecaImperialThermsInOneJoule and DecaUsThermsInOneJoule. What would say about it?

The C# files get replaced when building so that would unfortunately not work. The "simplest" solution would be to add the deca units manually to the json file.

eriove · 2017-09-17T17:34:33Z

UnitsNet/UnitDefinitions/Energy.json

@@ -89,6 +89,57 @@
          "Abbreviations": [ "Wh" ]
        }
      ]
+    },
+    {
+      "SingularName": "EuTherm",


This is called EC Therms in the Wikipedia article, probably because the unit is from the time when EU was called EC. Is EU Therm the most common name right now?

Well, I wish I could tell. From my experience people just use therm implicitly meaning a US or EU unit, that they solely use everywhere in documentation across a project. So looks like it is up to us how to call it.

I would go with calling it EC therm then to match Wikipedia. Then we are consistent with at least one other source. I'll leave the last word to @anjdreas

angularsen · 2017-09-18T11:15:56Z

A couple of things:

DecaEcTherm would break capitalization consistency with the rest of our units (Kilogram, Millimeter), unless we define that abbreviations like Ec or Us should keep their first character uppercase for prefixed units (deca, milli etc)
A possibly better naming scheme, is to put such notations as a suffix DecathermEc and ThermEc. This has the advantage of circumventing the whole special case for abbreviations since they won't be next to a unit prefix. It also better matches the textual representation in the wiki*
A final, minor point, is that you group together similar units, like BarrelImperial next to BarrelUs. If so, this should be made a code convention for future units - and add to our wishlist to change older units as part of a bigger breaking change.
EC vs EU: If I read it right, Wikipedia seems to prefer EC and I would need to hear a counter-argument from a domain expert to not go with the wikipedia definition.

https://en.wikipedia.org/wiki/Therm#Definitions *

Ten therms are known as a decatherm. Common abbreviations are Dth for decatherm (sometimes dekatherm[4]), MDth for a thousand decatherms and MMDth for a million decatherms.

DInozemtsev · 2017-09-19T09:29:59Z

As for me DecathermEc looks nice and neat. However it is not completely clear how would we name abbreviations. Should they be like this:
th (EC), th (US), th (UK),
Dth (EC), Dth (US), Dth (UK),
MDth (EC), MDth (US), MDth (UK),
MMDth (EC), MMDth (US), MMDth (UK) ?

angularsen · 2017-09-19T21:29:43Z

We have some sort of convention going with previous units, but it is not set in stone and probably not entirely consistent either. I'm losing a bit track of all the various units and their abbreviations.

Example: Volume unit abbreviations gal (U.S.), bl (U.S.), bl (imp.) etc.

I don't know what is the better choice here. I personally think UK and US reads better than U.S. and imp., but maybe it simply depends on the context and the conventions for each unit.
I guess I would prefer to use the same abbreviations that are used in the domain, and if there is no certain convention then maybe fall back to our (U.S.) and (imp.) convention for consistency?

cc @eriove @JKSnd

DInozemtsev · 2017-09-25T09:19:02Z

I would stay with already existing format, i mean like gal (U.S.) and sorry for delay, sirs, I was traveling at the end of last week. Hope will have enough time to implement this changes in a next few days.

DInozemtsev · 2017-09-25T11:31:39Z

Ok, that seems to be it. Sirs, could you plz review? I was in a bit rush, so maybe had missed something.

eriove · 2017-09-25T19:37:53Z

Looks good to me (I cross checked the numbers once more), @angularsen could you check to so that we didn't misunderstand each other.

angularsen

Found a minor discrepancy in the number.

angularsen · 2017-09-26T05:46:21Z

UnitsNet.Tests/CustomCode/EnergyTests.cs


        protected override double ElectronVoltsInOneJoule => 6.241509343260179e18;

        protected override double ErgsInOneJoule => 10000000;

-        protected override double EuThermsInOneJoule => 9.4781339449889105832843629746176e-9;
+        protected override double ThermsEcInOneJoule => 9.47816988e-9;


Any reason you shortened this?
Also I get a slightly different number here: 1J= 9.4781712e-9Therm(EC)

@eriove Mentioned this is probably more correct value. Please see the screenshot below.

Oh, okay. I have no familiarity with these units, why is there a different number in the calculator? Are there different standards for EC therms?

Second converter here:
http://www.kylesconverter.com/energy,-work,-and-heat/joules-to-therms-(e.c.)
9.478171203e-9

I think @eriove possibly found a value that was not entirely accurate.

From the last link:

1 Therm (E.C.):
Exactly 100,000 BTUIT. Approximately 1.05505585262 x 108 Joules (SI). Therm (E.C.) is based on the international table BTU, see Therm (U.S.) for Therms based on 59 °F BTU.

I took the number from EC Therms on the Wikipedia page which links to the ISO BTU, but I have no familiarity either so @DInozemtsev have to decide.

Ok, I can explain how I got a previous value.
So according Wikipedia:
1 Th(E.C.) ≡ 100,000 BTU = 105,505,600 joules

To get 1 Joule in Therms (E.C.):

1/105,505,600 = 9.4781698791343777012784155532976e-9
(this is what I get at Windows 10 Calculator.)

I think we should go with this value. @angularsen what do you think about this?

I think your value 105,505,600 is a rounded value, please read the quote from wiki:

1 Therm (E.C.):
Exactly 100,000 BTUIT. Approximately 1.05505585262 x 108 Joules (SI). Therm (E.C.) is based on the international table BTU, see Therm (U.S.) for Therms based on 59 °F BTU

1 Th EC = 1e5 BTU exactly and approx 105,505,852.62 joules. Then doing 1/x gives me 9.4781712031331720001278504447561e-9, which I believe is the correct value as it also matches the two online converters in above comments. This value should be used in the test case. Also since this is an infinite value, I think we should use the expression x/105505585.262 in the JSON for FromBaseToUnitFunc, and x*105505585.262 for FromUnitToBaseFunc.

DInozemtsev · 2017-09-28T09:46:13Z

Ok, I've just pushed changes. Could you sirs, @angularsen @eriove, please review them?

angularsen · 2017-09-28T09:53:39Z

Looks good to me!

eriove · 2017-09-28T19:09:38Z

Looks good to me!

eriove · 2017-09-28T19:18:44Z

@angularsen the build does not seem to get triggered. Did I do something wrong with version stepping?

angularsen · 2017-09-29T05:28:53Z

@eriove Ah, I had to update AppVeyor to point to new github username, it's now building, but nuget push failed so I'll have to look at that in the evening.

angularsen · 2017-09-29T20:33:21Z

Nuget 3.75 is out. I had to build and push manually from my own computer. There seems to be something wrong with appveyor, I'm piggybacking on someone elses' forum post here:
http://help.appveyor.com/discussions/problems/8215-cannot-publish-to-nuget-get-400-bad-request-error

Dmitriy Inozemtsev added 4 commits September 14, 2017 14:11

Add Therm unit

fbfdb00

Typo fixed

121f6c0

Generated code and tests

6064445

Some code. formating

a09f66b

eriove suggested changes Sep 17, 2017

View reviewed changes

Add improved name scheme

1c4614e

angularsen requested changes Sep 26, 2017

View reviewed changes

Update for ThermsEC

15baeef

angularsen approved these changes Sep 28, 2017

View reviewed changes

eriove merged commit 0f4c5d0 into angularsen:master Sep 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add US Therm, EU Therm and UK Therm #287

Add US Therm, EU Therm and UK Therm #287

DInozemtsev commented Sep 14, 2017

eriove commented Sep 14, 2017

DInozemtsev commented Sep 14, 2017 •

edited

Loading

DInozemtsev commented Sep 16, 2017

eriove left a comment

eriove Sep 17, 2017

DInozemtsev Sep 18, 2017

eriove Sep 17, 2017

angularsen Sep 17, 2017

angularsen Sep 17, 2017

DInozemtsev Sep 18, 2017

eriove Sep 18, 2017

eriove Sep 17, 2017

DInozemtsev Sep 18, 2017

eriove Sep 18, 2017

angularsen commented Sep 18, 2017

DInozemtsev commented Sep 19, 2017 •

edited

Loading

angularsen commented Sep 19, 2017 •

edited

Loading

DInozemtsev commented Sep 25, 2017

DInozemtsev commented Sep 25, 2017

eriove commented Sep 25, 2017 •

edited

Loading

angularsen left a comment

angularsen Sep 26, 2017

DInozemtsev Sep 26, 2017

angularsen Sep 26, 2017

eriove Sep 26, 2017

DInozemtsev Sep 27, 2017

angularsen Sep 27, 2017

DInozemtsev commented Sep 28, 2017

angularsen commented Sep 28, 2017

eriove commented Sep 28, 2017

eriove commented Sep 28, 2017

angularsen commented Sep 29, 2017

angularsen commented Sep 29, 2017

Add US Therm, EU Therm and UK Therm #287

Add US Therm, EU Therm and UK Therm #287

Conversation

DInozemtsev commented Sep 14, 2017

eriove commented Sep 14, 2017

DInozemtsev commented Sep 14, 2017 • edited Loading

DInozemtsev commented Sep 16, 2017

eriove 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

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

angularsen commented Sep 18, 2017

DInozemtsev commented Sep 19, 2017 • edited Loading

angularsen commented Sep 19, 2017 • edited Loading

DInozemtsev commented Sep 25, 2017

DInozemtsev commented Sep 25, 2017

eriove commented Sep 25, 2017 • edited Loading

angularsen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DInozemtsev commented Sep 28, 2017

angularsen commented Sep 28, 2017

eriove commented Sep 28, 2017

eriove commented Sep 28, 2017

angularsen commented Sep 29, 2017

angularsen commented Sep 29, 2017

DInozemtsev commented Sep 14, 2017 •

edited

Loading

DInozemtsev commented Sep 19, 2017 •

edited

Loading

angularsen commented Sep 19, 2017 •

edited

Loading

eriove commented Sep 25, 2017 •

edited

Loading