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

#59: Added missing units and updated number of units in test #75

Merged
merged 1 commit into from
Mar 21, 2017

Conversation

magnonasc
Copy link

@magnonasc magnonasc commented Mar 20, 2017

No description provided.

@magnonasc magnonasc self-assigned this Mar 20, 2017
// public static final Unit<Level<?>> NEPER = addUnit(new ProductUnit<Level<?>>(ONE.multiply(Math.log(1))));
// public static final Unit<Level<?>> BEL = addUnit(new ProductUnit<Level<?>>(ONE.multiply(Math.log10(1))));

// public static final Unit<Level<Pressure>> BEL_SOUND = addUnit(
Copy link
Author

@magnonasc magnonasc Mar 20, 2017

Choose a reason for hiding this comment

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

I'll start saying that this block is wrong, but I didn't know exactly how to make them, if I just use values, quantities (as some of them is in a weird unit as this one 2lg(2 10*-5.Pa)), and they can't be produced because I can't cast a TransformedUnit in a ProductUnit, I'll see it later. The other complication I had is when I have a multiplication by a number between 2 and 10, I have to use this unit ONE and keep multiplying it all over the place, isn't it a good idea to create something more flexible?

@@ -723,8 +738,11 @@ public static UCUM getInstance() {
// OTHER LEGACY UNITS: UCUM 4.5 §43 //
// ////////////////////////////////////
/** As per <a href="http://unitsofmeasure.org/">UCUM</a> standard. */
public static final Unit<Temperature> FAHRENHEIT = addUnit(KELVIN
.multiply(5).divide(9).shift(459.67));
Copy link
Author

Choose a reason for hiding this comment

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

@keilw, I didn't understand why you inverted this, any specific reason?

// SECTIONS §44-§46 skipped; implement later if needed //
// ///////////////////////////////////////////////////////

////////////////////////////////////////////
Copy link
Member

Choose a reason for hiding this comment

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

I note that the other (historical) comments put a space after the first //. Totally inconsequential! I just noticed it. Less than trivial.

Copy link
Author

@magnonasc magnonasc Mar 20, 2017

Choose a reason for hiding this comment

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

hahaha, yeeah, you're right.. I'm trying to not use any formatter for this project as everything would be a mess on diff. And because of that I forget some little things like this.

Copy link
Member

Choose a reason for hiding this comment

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

Honestly no idea where it first came up. It is not in JScience 5, so could be a formatting tool trying to expand or reduce the number of characters. This can be fixed wherever you spot it. Already did for most of the other unit systems like SI or implementation classes ;-)

@keilw keilw added this to the 0.7 milestone Mar 20, 2017
@keilw keilw mentioned this pull request Mar 20, 2017
Copy link
Member

@garretwilson garretwilson left a comment

Choose a reason for hiding this comment

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

I haven't compared this to the actual UCUM specification, but overall the code looks good I think. @keilw can answer your specific questions, because he's the expert in this area.

@keilw
Copy link
Member

keilw commented Mar 20, 2017

Yes, happy to help where I can. I requested a few UCUM units to be added over the past 7 years and some are now in the catalog.

@magnonasc
Copy link
Author

magnonasc commented Mar 20, 2017

Note that the units are in comments because, or we don't have a specific unit for that, or I didn't know how to implement (the case I asked your opinion), or there's no definition yet, which is good to at least keep there as comment.

@keilw
Copy link
Member

keilw commented Mar 20, 2017

Some units e.g. @dautelle defined as Dimensionless earlier like NEPER and BEL have the option of using a Level quantity (https://github.com/unitsofmeasurement/uom-systems/blob/master/quantity/src/main/java/systems/uom/quantity/Level.java) I did not start using it outside ISO 80000 (which is not public as such) but since UCUM is at least heavily "inspired" by ISO80k (and unless we're told otherwise it is perfectly fine and legal to reproduce the UCUM catalog) we might as well use it for http://unitsofmeasure.org/ucum.html#levels
For all that have no definition value or unit, I'd use the pattern of ONE in https://github.com/unitsofmeasurement/uom-se/blob/master/src/main/java/tec/uom/se/AbstractUnit.java.
public static final Unit<Dimensionless> INTERNATIONAL_UNIT = new ProductUnit<>(); The international unit is another case where two identical names exist with different manifestations. To be honest I don't know how to properly fix it, but looking at the description after http://unitsofmeasure.org/ucum.html#chemical, I'd try to solve it somehow by giving one a postfix like "WHO" or similar. We can think about it.

@magnonasc
Copy link
Author

Okay.. that might be helpful, thank you.

@keilw
Copy link
Member

keilw commented Mar 20, 2017

I'll have to look it up in the ISO80k vault, but e.g. pressure level would manifest itself as Level<Pressure>>.

@keilw
Copy link
Member

keilw commented Mar 20, 2017

I just created #76 for including the terms of use somehow in the UCUM bundle (probably as additional license beside BSD for the actual code)

We may have to be careful to watch e.g.

Users shall not use any of the UCUM codes in a way that expressly or implicitly changes their meaning

but I assume if there are two identical UCUM codes like "litre" and you must expose them by a unique name in Java, there is a slight freedom of adding e.g. "LITRE_A" and "LITRE_B" backed by the exact unchanged values to resolve such redundancies?;-)

@keilw keilw merged commit daabb89 into master Mar 21, 2017
@keilw
Copy link
Member

keilw commented Mar 21, 2017

Merge worked, thanks.
I saw the UCUM class now has an tab intent of 8 characters. Is that caused by the .gitattributes file, or a local editor or IDE format? It is not a big deal, just different from all the other modules with 4 chars only.

@magnonasc
Copy link
Author

magnonasc commented Mar 21, 2017

I use tab characters to indent instead of spaces, and if you see something changed on indentation is because there are things like this in the code:

    public void method(...) {
  a.very.bad.indented.code.in.some.places();
    for(...) {
  a.even.worse.indentation();
    }
    }

I use the formatter for Globalmentor, I apply it only to when I edit, but if you have that you like you can just provide me a link and I'll use it to this project, whatever you prefer.

@keilw
Copy link
Member

keilw commented Mar 21, 2017

I pushed it again, this time formatting based on an Eclipse setting called MicroProfile
based on https://github.com/eclipse/microprofile-samples/tree/master/src/format
The same repo has another file
https://github.com/eclipse/microprofile-samples/blob/master/.editorconfig
Have you tried that somewhere? Looks like it controls things like intents etc., too

@keilw
Copy link
Member

keilw commented Mar 21, 2017

Oh and I added NEPER based on how it worked elsewhere. To avoid accidentially mixing ProductUnit and other units, it is better to use a UnitConverter chain. And e.g. new LogConverter(1) instead of multiplying with Math operations.

@magnonasc
Copy link
Author

I never tried that .editorconfig, just the formatter.

And thank you about the units, I said that it was in a block of comments because I wasn't sure of how to implement that, but after that I think I'm ready to make some more units available.

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

Successfully merging this pull request may close these issues.

3 participants