Move to jsr363 dependencies #1834

dispiste · 2018-03-20T10:15:17Z

Proposal: https://github.com/geotools/geotools/wiki/Migrate-Units-to-JSR-363

https://osgeo-org.atlassian.net/browse/GEOT-5867

…f of concept

jodygarnett · 2018-03-20T10:48:33Z

Thank you, I will continue here for a bit...

And start of manually fixing compile errors

jodygarnett · 2018-03-21T10:20:28Z

modules/library/referencing/pom.xml

+    <dependency>
+      <groupId>si.uom</groupId>
+      <artifactId>systems-common</artifactId>
+      <version>0.7.2</version>


we should get this from dependency management

into test_units_jsr363

…ation

jodygarnett · 2018-03-26T23:36:36Z

So I am running into trouble with gt-coverage which makes use of JSCR-275 Measure:

http://jscience.org/api/javax/measure/Measure.html

With Measure<V,Q extends Quantity>

This class represents the result of a measurement stated in a known unit.
There is no constraint upon the measurement value itself: scalars, vectors, or even data sets are valid values as long as an aggregate magnitude can be determined (see Measurable).

Examples provided for:

DecimalMeasure - a measure whose value is an arbitrary-precision decimal number
VectorMeasure - a measurement vector of two or more dimensions

I cannot seem to find a similar JSR-363 representation.

Further research shows:

Vector Quantity Support unitsofmeasurement/unit-ri#61 Vector Quantity Support from someone else making the migration from JScience
points to our friends at apache sis who have the same need ( https://github.com/apache/sis/blob/trunk/core/sis-utility/src/main/java/org/apache/sis/measure/Scalar.java)

aaime · 2018-04-14T15:41:11Z

Too big, cannot finish it today. I'll try to resume tomorrow (already set various comments in the review, but it's not complete)

aaime

Overall looks good, most of the changes are mechanic search and replace. Found some duplicated code and have some questions.

aaime · 2018-04-14T14:58:09Z

modules/extension/xsd/xsd-sld/src/test/java/org/geotools/se/v1_1/RountTripLocalUomTest.java

@@ -14,7 +15,8 @@
    @Test
    public void testRoundTripPoint() throws Exception {
        PointSymbolizer sym = (PointSymbolizer) parse("example-pointsymbolizer-local-uom.xml");
-        assertEquals(NonSI.PIXEL, sym.getUnitOfMeasure());
+        assertEquals("pixel", Units.toName( sym.getUnitOfMeasure() ));


So there is no constant for pixels? What's the deal here?

There is a new Units.PIXEL, would be nice if it was used consistently

Strangely we did find a PIXEL constant, as a unit of information measuring 4 bytes :)

aaime · 2018-04-14T14:59:24Z

modules/extension/ysld/src/test/java/org/geotools/ysld/encode/YsldEncodeTest.java

@@ -1317,7 +1317,7 @@ public void testSymbolizerUoMPixel() throws Exception {
    public void testSymbolizerUoMOther() throws Exception {
        PointSymbolizer p = CommonFactoryFinder.getStyleFactory().createPointSymbolizer();
        FeatureTypeStyle fts = fts(p);
-        p.setUnitOfMeasure(NonSI.LIGHT_YEAR);
+        p.setUnitOfMeasure(NonSI.ASTRONOMICAL_UNIT);


Not sure if relevant in the context of the test, but an "astronomical unit" is not a light year: https://en.wikipedia.org/wiki/Astronomical_unit

aaime · 2018-04-14T15:03:31Z

.../src/main/java/org/geotools/coverage/grid/io/imageio/geotiff/CRS2GeoTiffMetadataAdapter.java

@@ -970,13 +979,15 @@ private void parseUnit(Unit<?> unit, int model,GeoTiffIIOMetadataEncoder metadat
                default:
                    throw new IllegalStateException("Unable to map model "+model+" for this unit");
 	        }
-	        if(unit.equals(SI.METER)){
+	        unit = Units.autoCorrect(unit); // auto-correct DEGREE_ANGLE and FOOT_SURVEY


Seems like a workaround. Is there any guidance of where this should be used? I see it applied in various places, seems like "ah hoc", "if it breaks then I'll add it" style.

You are correct, so far it has just added it when running into accuracy differences that prevented "matching" units from being recognized as such.

Perhaps Units.toCanonicalUnit(Unit) would be more appropriate.

aaime · 2018-04-14T15:03:44Z

modules/library/referencing/src/main/java/org/geotools/measure/Units.java

+        format.label(Units.PIXEL, "pixel");
+
+        format.label(tec.uom.se.unit.Units.KELVIN, "kelvin");
+        //format.alias(tec.uom.se.unit.Units.KELVIN, "kelvin");


Leftover commented out code

aaime · 2018-04-14T15:06:45Z

.../src/main/java/org/geotools/coverage/grid/io/imageio/geotiff/CRS2GeoTiffMetadataAdapter.java

-                            }
+                        if (!AbstractUnit.ONE.equals(unit)) {
+                            // or SI.RADIAN
+                            converter = ((Unit<Angle>) unit).getConverterTo(NonSI.SECOND_ANGLE);


Confused, base was RADIAN and now it has become SECOND_ANGLE?

Base was compatible with radian, was not necessarirly radian. Still I will run test cases again and see if I can make the code more clear

aaime · 2018-04-14T15:13:47Z

modules/library/render/src/main/java/org/geotools/renderer/windbarbs/SpeedConverter.java

@@ -38,7 +41,7 @@
            .getLogger(SpeedConverter.class);

    /** UCUM_FORMAT_INSTANCE */
-    private static final UnitFormat UCUM_FORMAT_INSTANCE = UnitFormat.getUCUMInstance();
+    //private static final UnitFormat UCUM_FORMAT_INSTANCE = UCUMFormat.getInstance(Variant.CASE_INSENSITIVE);


Leftover commented out block

aaime · 2018-04-14T15:18:13Z

modules/library/metadata/src/main/java/org/geotools/util/MeasurementRange.java

    @Override
    public <N extends Number & Comparable<? super N>> MeasurementRange<N> castTo(Class<N> type) {
-        return (MeasurementRange) damnJava5(this, type);
+        return (MeasurementRange<N>) this;


LOL, this old method made me laugh quite a bit :-)

aaime · 2018-04-14T15:25:38Z

modules/library/metadata/src/main/java/org/geotools/util/MeasurementRange.java

-        if (converter.equals(UnitConverter.IDENTITY)) {
+        UnitConverter converter;
+		try {
+			converter = units.getConverterToAny(targetUnits);


Bit confused, there is a getConverterTo method in Units that seems to be already doing the same job as this try catch, or not? https://github.com/unitsofmeasurement/unit-api/blob/master/src/main/java/javax/measure/Unit.java#L184

Btw, seen this block repeated somewhere else too (GeneralEnvelope for example), actually, in many places where getConverterToAny is used. If getConverterTo cannot be used for some reason, probably best to factor it out in Units?

aaime · 2018-04-14T15:33:21Z

modules/library/referencing/src/main/java/org/geotools/referencing/cs/DefaultEllipsoidalCS.java

+            final Unit<?> unit = axis.getUnit();
+            try {
+                if (AxisDirection.EAST.equals(direction)) {
+                    longitudeAxis = i;


Inconsistent formatting, why the extra new lines here and not in the other blocks?

aaime · 2018-04-14T18:02:25Z

modules/library/referencing/src/main/java/org/geotools/referencing/wkt/GeoToolsUnitFormat.java

-                    label(unit, name);
-                }
-            }
+                java.lang.reflect.Field nameToUnitField = DefaultFormat.class


This is... unavoidable but bad. Was a improvement request filed against the implementation to make DefaultFormat/SimpleUnitFormat clonable (maybe by copy constructor) to allow building variants of it without resorting to reflection in the future?

aaime · 2018-04-14T18:19:11Z

One thing that would be useful for all other devs that were not involved in the sprint is a short guide of how to do things past migration. Most of the info seems to be contained in the proposal, but would be have a table and maybe some indication about the usage of the "magic" autocorrect method (when to use it).

…t SimpleUnitFormat which ensures that Geotools custom units are correctly registered. Adapt GeoToolsUnitFormat accordingly

dispiste · 2018-04-17T10:04:23Z

Hi @jodygarnett @aaime

I've committed some additional fixes, but I there are some fundamental issues (in addition to the minor corrections pointed by @aaime in the review) which IMHO should be solved before merging (or at least they should be considered when approving the migration). Unfortunately I will not be able to invest more time in this JSR363 migration (I've already spent quite a lot), so I've taken some time to explain the issues and I've also outlined 2 different solutions to avoid them (one of them is implemented in a new subbranch, see details below).

The root of the problem: the old JSR275 library was able to identify equivalent unit definitions in the equals method, while the new one is not able to do so when the units to compare are defined using different converter classes (e.g. RationalConverter vs MultiplyConverter).

This different behaviour has an impact at least in the following Geotools use cases, potentially causing regressions:

method CRS.lookupEpsgCode(crs, false): when trying to derive an EPSG code from a CRS created from a non-EPSG WKT string, because the crs units are not considered equivalent units to the ones defined in the EPSG database and the replacement fails
method CRS.toWKT(): when trying to format a unit. In the resulting WKT string, units that have not been recognised will be assigned a name based in the base unit and the conversion factor. Example using JSR-363 implementation: UNIT["m*0.3048006096012192", 0.3048006096012192], instead of UNIT["foot_survey_us", 0.3048006096012192] using the old JSR-275 library.
when parsing/writing units for Geotiff reading/writing (see CRS2GeoTiffMetadataAdapter.parseLinearUnit and parseUnit methods)
in any place where the getConverterTo()/getConverterToAny() method is used: it will produce a non-Identity converter if the fromUnit and toUnit aren't considered equivalent by the new library. This creates precision errors as illustrated in the following examples:

// old lib: returns 1.0
NonSI.DEGREE_ANGLE.getConverterTo(SI.RADIAN.times(0.0174532925199433)).convert(1.0)
// new lib: returns 0.9999999999999998
NonSI.DEGREE_ANGLE.getConverterTo(SI.RADIAN.multiply(0.0174532925199433)).convert(1.0)
// old lib: returns 23.0
NonSI.DEGREE_ANGLE.getConverterTo(SI.RADIAN.times(0.0174532925199433)).convert(23.0)
// new lib: returns 22.999999999999993
NonSI.DEGREE_ANGLE.getConverterTo(SI.RADIAN.multiply(0.0174532925199433)).convert(23.0)

The behaviour of the new implementation can't be considered incorrect (it is more precise), but Geotools is not expecting this behaviour and it will produce different results and even errors in some situations.

In order to mitigate these issues, Units.autocorrect method was introduced, which tries to replace the provided unit by another well-known unit. This is done case by case (currently only for 2 units), so it mitigates the problem but does not prevent it to happen for other units that have not been considered in autocorrect.

We can envisage a full solution to these issues using one of the following approaches:

We can't inject the old behaviour in the JSR363 implementation, but we can implement a Units.equals(Unit unit1, Unit unit2) method which mimics the JSR-275 behaviour and use it in key points of Geotools. In addition, we will also override the forName method in EPSGFormat and ESRIFormat classes to use equivalent names when available. Finally, we will provide an Units.getConverterToAny method which provides the Identity converter if units are considered equivalent. Note that this approach might still fail if we have failed to identify some of the key points in which Units.equals should be used. I've created a subbranch implementing this solution, which compiles and passes tests (even disabling the autocorrect method), but I find the changes quite intrusive. On the TODO list: replace all the usages of getConverterTo()/getConverterToAny() by the Units.getConverterToAny() method. The branch is here: units_jsr363_equalsapproach
We can try to get one of the units defined by the new implementation whenever we need to instantiate a unit. We would parse the unit name to get a unit instance, then we would compare the conversion factor of the instantiated unit with the provided provided factor. In this way, we will not need to later check for units equivalences and we would avoid all the mentioned issues. The will produce a side effect when we parse a unit such UNIT["degree", 0.0174532925199433], since it will immediately be converted on the reference degree unit and we will loose some information (the real conversion factor defined in the WKT). Anyway, this is partially happening already in the JSR275 implementation and also in the S1 solution, so I think it is acceptable. I find this approach cleaner and simpler and I'm trying to implement it on a new subbranch.

Finally, note that some small differences will remain compared with the JSR275 implementation:

Units may have a different label, which has some effects in formatting. For instance, old library defined "foot_survey_us" while the new one defines "ft_survey_us". It doesn't seem a big problem, since none of these labels is the official one defined by the EPSG database: "US Survey foot". In general, names seem to be ignored when parsing units. In any case, we can override labels in Units.registerCustomUnits (system-wide), in the EPSGFormat constructor (epsg-wide) or in the ESRIFormat constructor (esri-wide)

I hope this information is useful to take a decision for next steps.

…ethod which mimics JSR275 behaviour

dispiste · 2018-04-17T17:50:03Z

Hi again, I've finally done a last try and implemented an approach similar to the second solution outlined above. Basically, it consists on replacing the existing autocorrect method by a more generic one which mimics behaviour of the old JSR275 library. I also tried to use autocorrect only where necessary (for units instantiation), so I removed it from some places. I am quite satisfied with the result, so I've committed it to this branch. It compiles and passes tests.

I believe that now only @aaime comments needs to be tackled to get the PR ready for merging.

I've written some documentation regarding units usage, which might be added to the proposal:

JSR363 Units HOWTO

Getting Unit instances

If you know the unit to use at compile time, use one of the Unit instances defined as static variables in org.geotools.measure.Units, si.uom.SI, si.uom.NonSI or systems.uom.common.USCustomary.

If you need to define new Units at runtime, it is important to immediately try to convert the new unit to one of the existing instances using Units.autocorrect method. Autocorrect applies some tolerance to locate an equivalent Unit. Skipping autocorrect will produce unexpected results and errors due to small differences in units definition. Examples:

// the result should be NonSI.DEGREE_ANGLE:
Unit<?> deg = Units.autoCorrect(SI.RADIAN.multiply(0.0174532925199433));

Unit<?> halfMetre = SI.METRE.divide(2);
// the result should be SI.METRE
Unit<?> stupidUnit = Units.autocorrect(halfMetre.multiply(4).divide(2));

public <T extends Quantity<T>> Unit<T> deriveUnit(Unit<T>  baseUnit, double factor) {
    return Units.autocorrect(baseUnit.multiply(factor);)
}

Use a specific Quantity whenever possible to get type-safety checks at compile time:

Unit<Length> halfMetre = SI.METRE.divide(2);
Unit<Length> stupidUnit = Units.autocorrect(halfMetre.multiply(4).divide(2));

Formatting units

Use org.geotools.measure.Units.toName() to get the unit name (or unit label if name is not defined). Use org.geotools.measure.Units.getDefaultFormat().format() to get the unit label (ignoring the name). Examples:

Unit<?> unit = ...
System.out.println(Units.toName(unit)):

// prints "Litre"
System.out.println(Units.toName(SI.LITRE))
// prints "l"
System.out.println(Units.getDefaultFormat().format(SI.LITRE))
// Most units don't define a name, so it does not make a difference
// prints "m"
System.out.println(Units.toName(SI.METRE))
// prints "m"
System.out.println(Units.getDefaultFormat().format(SI.METRE))

There are specific formatters for EPSG and ESRI dialects (although they are not public classes, they are used by WKT Formatter class), which override the default formatting (for instance EPSG formatter prints "Metre" instead of "m").
Labels can be overridden in Units.registerCustomUnits (system-wide), in the EPSGFormat.epsgLabelsAndAliases mehtod (epsg-wide) or in the ESRIFormat.esriLabelsAndAliases constructor (esri-wide)

Converting units

If the unit Quantity type is known, use the type-safe getConverterTo() method:

Unit<Angle> unit = ...
UnitConverter converter = unit.getConverterTo(SI.RADIAN);
double convertedQuantity = converter.convert(3.1415);

If the Quantity type is undefined, use the convenience method org.geotools.measure.Units.getConverterToAny(). Note that this method throws an IllegalArgumentException if units can't be converted:

Unit<?> unit = ...
UnitConverter converter = Units.getConverterToAny(unit, SI.RADIAN);
double convertedQuantity = converter.convert(3.1415);

aaime · 2018-04-17T18:01:16Z

On Tue, Apr 17, 2018 at 12:04 PM, Cesar Martinez Izquierdo < ***@***.***> wrote: Hi @jodygarnett <https://github.com/jodygarnett> @aaime <https://github.com/aaime> I've committed some additiona fixes, but I there are some fundamental issues (in addition to the minor corrections pointed by @aaime <https://github.com/aaime> in the review) which IMHO should be solved before merging (or at least they should be considered when approving the migration). Unfortunately I will not be able to invest more time in this JSR363 migration (I've already spent quite a lot), so I've taken some time to explain the issues and I've also outlined 2 different solutions to avoid them (one of them is implemented in a new subbranch, see details below). The root of the problem: the old JSR275 library was able to identify equivalent unit definitions in the equals method, while the new one is not able to do so when the units to compare are defined using different converter classes (e.g. RationalConverter vs MultiplyConverter). This different behaviour has an impact at least in the following Geotools use cases, potentially causing regresions: 1. method CRS.lookupEpsgCode(crs, false): when trying to derive an EPSG code from a CRS created from a non-EPSG WKT string, because the crs units are not considered equivalent units to the ones defined in the EPSG database and the replacement fails Yep, this is serious, and it's going to make a sizeable amount of work I

did in this area regress, but maybe not a stopper

1. method CRS.toWKT(): when trying to format a unit. In the resulting WKT string, units that have not been recognised will be assigned a name based in the base unit and the conversion factor. Example using JSR-363 implementation: UNIT["m*0.3048006096012192", 0.3048006096012192], instead of UNIT["foot_survey_us", 0.3048006096012192] using the old JSR-275 library. This seem like a stopper, but I'm not sure, afraid it will cause issues

with shapefile/world files we generate (or else, with other systems ability to deal with it).

1. in any place where the getConverterTo()/getConverterToAny() method is used: it will produce a non-Identity converter if the fromUnit and toUnit aren't considered equivalent by the new library. This creates precision errors as illustrated in the following examples: // old lib: returns 1.0NonSI.DEGREE_ANGLE.getConverterTo(SI.RADIAN.times(0.0174532925199433)).convert(1.0)// new lib: returns 0.9999999999999998NonSI.DEGREE_ANGLE.getConverterTo(SI.RADIAN.multiply(0.0174532925199433)).convert(1.0)// old lib: returns 23.0NonSI.DEGREE_ANGLE.getConverterTo(SI.RADIAN.times(0.0174532925199433)).convert(23.0)// new lib: returns 22.999999999999993NonSI.DEGREE_ANGLE.getConverterTo(SI.RADIAN.multiply(0.0174532925199433)).convert(23.0) The behaviour of the new implementation can't be considered incorrect (it is more precise), but Geotools is not expecting this behaviour and it will produce different results and even errors in some situations.

Unsure about this one (if it's a stopper or not)

In order to mitigate these issues, Units.autocorrect method was introduced, which tries to replace the provided unit by another well-known unit. This is done case by case (currently only for 2 units), so it mitigates the problem but does not prevent it to happen for other units that have not been condidered in autocorrect. We can envisage a full solution to these issues using one of the following approaches: 1. We can't inject the old behaviour in the JSR363 implementation, but we can implement a Units.equals(Unit unit1, Unit unit2) method which mimics the JSR-275 behaviour and use it in key points of Geotools. In addition, we will also override the forName method in EPSGFormat and ESRIFormat classes to use equivalent names when available. Finally, we will provide an Units.getConverterToAny method which provides the Identity converter if units are considered equivalent. Note that this approach might still fail if we have failed to identify some of the key points in which Units.equals should be used. I've created a subbranch implementing this solution, which compiles and passes tests (even disabling the autocorrect method), but I find the changes quite intrusive. On the TODO list: replace all the usages of getConverterTo()/getConverterToAny() by the Units.getConverterToAny() method. The branch is here: units_jsr363_equalsapproach Interesting, I'll try to have a look 1. We can try to get one of the units defined by the new implementation whenever we need to instantiate a unit. We would parse the unit name to get a unit instance, then we would compare the conversion factor of the instatiated unit with the provided provided factor. In this way, we will not need to later check for units equivalences and we would avoid all the mentioned issues. The will produce a side effect when we parse a unit such UNIT["degree", 0.0174532925199433], since it will inmediately be converted on the reference degree unit and we will loose some information (the real conversion factor defined in the WKT). Anyway, this is partially happening alredy in the JSR275 implementation and also in the S1 solution, so I think it is acceptable. I find this approach cleaner and simpler and I'm trying to implement it on a new subbranch. Yep, agreed. Finally, note that some small differences will remain compared with the JSR275 implementation: - Units may have a different label, which has some effects in formatting. For instance, old library defined "foot_survey_us" while the new one defines "ft_survey_us". It doesn't seem a big problem, since none of these labels is the official one defined by the EPSG database: "US Survey foot". In general, names seem to be ignored when parsing units. In any case, we can override labels in Units.registerCustomUnits (system-wide), in the EPSGFormat constructor (epsg-wide) or in the ESRIFormat constructor (esri-wide) Yeah, I guess it would be useful.

I am worried about the current state, as this is crossing with the "wholesale reformat" proposal, if this does not get merged before the reformat goes in, you'll get a landslide of conflicts. Thankfully the reformat proposal is a "method and tool" that requires very few manual changes, the reformat will be done on the fly when we are ready by a tool... but I'd rather not wait for months before applying it (I'm happy to wait a few weeks more if needed though). Cheers Andrea == GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information. == Ing. Andrea Aime @geowolf Technical Lead GeoSolutions S.A.S. Via di Montramito 3/A 55054 Massarosa (LU) phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 http://www.geo-solutions.it http://twitter.com/geosolutions_it AVVERTENZE AI SENSI DEL D.Lgs. 196/2003 Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003. The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

jodygarnett · 2018-04-17T18:54:28Z

Thanks for they key update @dispiste, I too am quite happy with the "second approach" result.

dispiste · 2018-04-17T19:09:58Z

Hi, I've tackled the requested changes. I've ignored code formatting concerns (they are the result of a lost battle against configuration of Eclipse automatic format), since I understand that a global reformatting is coming soon.

Maybe commit 7aba004 needs to be also reviewed, then we are ready to merge IMHO.

jodygarnett

This looks good, maybe missing one header.

jodygarnett · 2018-04-20T08:14:01Z

modules/library/referencing/src/main/java/org/geotools/geometry/GeneralEnvelope.java

-                } catch (UnconvertibleException | IncommensurableException e) {
-                    throw new IllegalArgumentException(e);
-                }
+                value = Units.getConverterToAny(source, unit).convert(value);


This change makes sense

jodygarnett · 2018-04-20T08:14:48Z

modules/library/referencing/src/main/java/org/geotools/measure/Units.java

@@ -218,7 +218,7 @@ public static UnitConverter getConverterToAny(Unit<?> fromUnit, Unit<?> toUnit)
        try {
            return fromUnit.getConverterToAny(toUnit);
        } catch (UnconvertibleException | IncommensurableException e) {
-            throw new IllegalArgumentException(e);
+            throw new IllegalArgumentException("Can't convert to the candidate unit", e);


any chance of using the unit names or labels here so the error message is not so generic?

jodygarnett · 2018-04-20T08:18:29Z

modules/library/referencing/src/main/java/org/geotools/measure/Units.java

+                                .concatenate(tunit2.getSystemConverter().inverse()).convert(1.0f);
+                        // FIXME: old JSR-275 library converted to float to compare factors to provide some tolerance
+                        // Should we use a configurable tolerance or a smaller tolerance using doubles?
+                        if (factor == 1.0f) {


You could down grade this comment to a "note" rather than a "fixme" since the code is reproducing the behaviour you intended. You are matching the old JSR-275 functionality, float just being a rather amusing way to specify a tolerance.

I am quite impressed with the generic nature of this implementation.

jodygarnett · 2018-04-20T08:20:05Z

modules/library/referencing/src/main/java/org/geotools/referencing/wkt/DefaultUnitParser.java

@@ -0,0 +1,112 @@
+package org.geotools.referencing.wkt;


Do we meed a header here?

jodygarnett · 2018-04-20T20:54:13Z

I have written up your notes and the change proposal as as upgrade documentation.

This is now ready to merge in conjunction with geoserver/geoserver#2828

jodygarnett · 2018-04-21T01:09:06Z

Okay so geoserver reports a netCDF regression:

Failed tests:   testRapNativeGribRotatedPole(org.geoserver.wcs2_0.WCSNetCDFTest): expected:<[K]> but was:<[kelvin]>
  testNetcdfRotatedPole(org.geoserver.wcs2_0.WCSNetCDFTest): expected:<[K]> but was:<[kelvin]>

Difference between using a symbol K and a name kelvin? Or do we need to make another format / configuration for use with netCDF.

Research:

https://www.unidata.ucar.edu/software/netcdf/netcdf/Units.html

jodygarnett · 2018-04-21T01:13:05Z

...ugin/coverage-multidim/netcdf/src/main/java/org/geotools/imageio/netcdf/VariableAdapter.java

@@ -880,8 +882,9 @@ private void initRange() {
                for (UnitCharReplacement replacement: UNIT_CHARS_REPLACEMENTS) {
                    unitString = replacement.replace(unitString);
                }
-                unit = Unit.valueOf(unitString);
-            } catch (IllegalArgumentException iae) {
+                unit = SimpleUnitFormat.getInstance().parse(unitString);


May need to use Units.parseUnit(unitStirng) to access configured units?

In Units [1], we define "kelvin" as the label for Kelvin unit. You might want to change it to "K".

Replacing SimpleUnitFormat.getInstance().parse(unitString); by Units.parseUnit() is also a good idea, since it ensures that our units are registered on SimpleUnitFormat (Geotools units are registered during static code execution of Units class).

[1] https://github.com/dispiste/geotools/blob/0b824095825e9287f7e2a16a983a1aa984d29fc2/modules/library/referencing/src/main/java/org/geotools/measure/Units.java#L136

dispiste · 2018-04-23T08:33:05Z

JSR363 API provides some methods in Unit interface:

getName(): it should get the name
getSymbol(): it should get the symbol

However, the implementation returns null for getName() and/or getSymbol() for most units. Some examples:
SI.METRE.getName() returns null
SI.METRE.getSymbol() returns "m"
SI.LITRE.getName() returns "Litre"
SI.LITRE.getSymbol() returns null
NonSI.DEGREE_ANGLE.getName() returns "Degree Angle"
NonSI.DEGREE_ANGLE.getSymbol() returns null
USCustomary.FOOT_SURVEY.getName() returns "US Survey foot"
USCustomary.FOOT_SURVEY.getSymbol() returns null

The toString() method relies on SimpleUnitFormat.getInstance().format() method.

The SimpleUnitFormat.getInstance().format() method is based on the labels that have been registered using the label() method. If no labels have been registered, format uses getSymbol() for BaseUnits and AlternateUnits. For TransformedUnits, a name is composed based on the BaseUnit format and the conversion factor.

Therefore, from a practical point of view, I would say we only have format() method, which we can configure in the best way we decide, but that it will fallback to getSymbol() to "unconfigured" units. Therefore, I'd consider it a replacement for getSymbol() and not for getName().

Note that there are 2 main use cases for names and symbols: formatting and parsing. We can only set one label for formatting, but we can use aliases to be able to parse alternate symbols or names.

…ress the differences between getSymbol and getName

dispiste · 2018-04-23T13:54:40Z

Hi again @jodygarnett, I've removed kelvin label, since symbol is already defined as "K" by default, and everything compiles and passes tests (in Geotools and also in Geoserver). I've used the following command:
mvn -Drelease clean install

Note that you'll need to sync the test_units_jsr363 branch with geotools master in order to successfully compile Geoserver.

We can further refine formatting if needed after merging the branch in master (for instance if we eventually need a custom formatter for NetCDF).

I've added a new convenience method to Units class:

public static String toSymbol(Unit<?> unit)

because as explained in my previous comment, we'll wish to get the symbol most of the time, since we can't declare/alter names with current implementation. If we really need to declare unit names (in addition to labels), we could create a mechanism to add names to DefaultUnitParser, then we could add a formatName() method in DefaultUnitParser and use this method in Units.toName().

jodygarnett · 2018-04-23T14:17:04Z

rebased and merged to master

jodygarnett · 2018-04-23T14:20:58Z

Thanks @dispiste I was rebasing to master concurrently, I have picked up your commit via cherry-pick

jodygarnett · 2018-04-23T15:53:07Z

@dispiste thanks again for you hard work, build.geoserver.org is reporting success across geotools and geoserver branches.

bencaradocdavies · 2018-04-24T03:46:39Z

@dispiste @jodygarnett while I concur that build.geoserver.org is passing, and my local build is passing, Travis CI fails for GeoServer master with a suspicious mention of kelvin:

testRapNativeGribRotatedPole(org.geoserver.wcs2_0.WCSNetCDFTest)  Time elapsed: 218 sec  <<< FAILURE!
org.junit.ComparisonFailure: expected:<[K]> but was:<[kelvin]>
	at org.junit.Assert.assertEquals(Assert.java:115)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at org.geoserver.wcs2_0.WCSNetCDFTest.testRapNativeGribRotatedPole(WCSNetCDFTest.java:284)

testNetcdfRotatedPole(org.geoserver.wcs2_0.WCSNetCDFTest)  Time elapsed: 175 sec  <<< FAILURE!
org.junit.ComparisonFailure: expected:<[K]> but was:<[kelvin]>
	at org.junit.Assert.assertEquals(Assert.java:115)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at org.geoserver.wcs2_0.WCSNetCDFTest.testNetcdfRotatedPole(WCSNetCDFTest.java:214)

bencaradocdavies · 2018-04-24T03:47:44Z

Note also that there is another unrelated failure in the mix. The WCSNetCDFTest failure is new and started after this PR was merged.

bencaradocdavies · 2018-04-24T03:57:49Z

I can confirm that this test is run locally in both Maven and Eclipse and passes in both cases.

bencaradocdavies · 2018-04-24T04:51:46Z

@dispiste @jodygarnett I re-ran the most recent GeoServer master build on Travis CI and it passed this time, so I guess the previous failure was just a temporary Travis CI problem: https://travis-ci.org/geoserver/geoserver/builds/370120346

dispiste · 2018-04-24T13:18:49Z

Thank you @jodygarnett for supporting and petting this transition, and also @aaime and the all the rest of the team for reviewing the changes and ensuring everything looks good.

…

On 24 April 2018 at 06:51, Ben Caradoc-Davies ***@***.***> wrote: @dispiste <https://github.com/dispiste> @jodygarnett <https://github.com/jodygarnett> I re-ran the most recent GeoServer master build on Travis CI and it passed this time, so I guess the previous failure was just a temporary Travis CI problem: https://travis-ci.org/ geoserver/geoserver/builds/370120346 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1834 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABfd3_lv6z81Kx15nm2kK4yJ9LQGeFPVks5trq9lgaJpZM4Sxnp9> .

-- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - César Martínez Izquierdo GIS developer - - - - - - - - - - - - - - - - - - - - SCOLAB: http://www.scolab.es - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Move to jsr363 dependencies and try to compile some classes as a proo…

9b4e8b6

…f of concept

jodygarnett added 4 commits March 20, 2018 11:52

fix unit import

04e586c

Search and replace to si.uom.SI

ee1a384

And start of manually fixing compile errors

fix iimport

15f3214

Change to MetricPrefix.KILO

aa8fb08

jodygarnett changed the title ~~Move to jsr363 dependencies and try to compile some classes as a proo…~~ Move to jsr363 dependencies Mar 21, 2018

jodygarnett and others added 3 commits March 21, 2018 10:17

gt-metadata now compiles

5c9cd2d

Add system-commons dependency to make US Customary units available

fd4a32c

Nautical miles, yards, foot_survey

dfe3d87

jodygarnett reviewed Mar 21, 2018

View reviewed changes

jodygarnett and others added 18 commits March 21, 2018 11:28

restore casts

142d5bc

gt-rendering converted

62fa48e

gt-gml3 converted

901d5bb

Fix SexigesimalConverter, CoordinateFormat and GeneralEnvelope

abf8637

Merge branch 'test_units_jsr363' of https://github.com/dispiste/geotools

34698a1

into test_units_jsr363

api conversion

337d92f

Fixes in referencing module

7d7c966

Merge branch 'test_units_jsr363' of https://github.com/dispiste/geotools

5f84eaa

into test_units_jsr363

gt-process migrated

48c9b1a

gt-netcdf migrationed

1a81992

More fixes in referencing module

315f04f

Merge branch 'test_units_jsr363' of https://github.com/dispiste/geotools

0e9b8e4

into test_units_jsr363

More fixes in referencing module

17d3527

Referencing module should compile now

5ba88de

gt-main converted

9e3ad6a

mbstyle migrated to geotools internal definition of pixel

4f2a6bb

Correct javadocs

523191b

resolve conflict with standard implementation and reference implement…

248ab9d

…ation

aaime requested changes Apr 14, 2018

View reviewed changes

dispiste added 3 commits April 16, 2018 18:31

Add getDefaultFormat() method in Units, as a way to access the defaul…

9f907f9

…t SimpleUnitFormat which ensures that Geotools custom units are correctly registered. Adapt GeoToolsUnitFormat accordingly

Fix compilation in unsupported module

91a0036

Use the same units that were originally used in the test

2f69cac

Implement a generic autoCorrect method which compares units using a m…

7aba004

…ethod which mimics JSR275 behaviour

Refactor getConvertionToAny usages and tackle PR change requests

8143b4c

jodygarnett approved these changes Apr 20, 2018

View reviewed changes

headers, comments and docs

0b82409

jodygarnett reviewed Apr 21, 2018

View reviewed changes

Remove unnecessary kelvin unit. Add toSymbol convenience method to st…

f4a744c

…ress the differences between getSymbol and getName

jodygarnett closed this Apr 23, 2018

Move to jsr363 dependencies #1834

Move to jsr363 dependencies #1834

Conversation

dispiste commented Mar 20, 2018 • edited by jodygarnett Loading

jodygarnett commented Mar 20, 2018

Choose a reason for hiding this comment

jodygarnett commented Mar 26, 2018 • edited Loading

aaime commented Apr 14, 2018

aaime 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

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

aaime commented Apr 14, 2018

dispiste commented Apr 17, 2018 • edited Loading

dispiste commented Apr 17, 2018

JSR363 Units HOWTO

Getting Unit instances

Formatting units

Converting units

aaime commented Apr 17, 2018 via email

jodygarnett commented Apr 17, 2018

dispiste commented Apr 17, 2018

jodygarnett left a comment • edited Loading

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

jodygarnett commented Apr 20, 2018

jodygarnett commented Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dispiste commented Apr 23, 2018

dispiste commented Apr 23, 2018

jodygarnett commented Apr 23, 2018

jodygarnett commented Apr 23, 2018

jodygarnett commented Apr 23, 2018

bencaradocdavies commented Apr 24, 2018 • edited Loading

bencaradocdavies commented Apr 24, 2018

bencaradocdavies commented Apr 24, 2018

bencaradocdavies commented Apr 24, 2018

dispiste commented Apr 24, 2018 via email

dispiste commented Mar 20, 2018 •

edited by jodygarnett

Loading

jodygarnett commented Mar 26, 2018 •

edited

Loading

dispiste commented Apr 17, 2018 •

edited

Loading

jodygarnett left a comment •

edited

Loading

jodygarnett commented Apr 21, 2018 •

edited

Loading

bencaradocdavies commented Apr 24, 2018 •

edited

Loading