[ 
https://issues.apache.org/jira/browse/IMAGING-266?focusedWorklogId=648401&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-648401
 ]

ASF GitHub Bot logged work on IMAGING-266:
------------------------------------------

                Author: ASF GitHub Bot
            Created on: 09/Sep/21 06:04
            Start Date: 09/Sep/21 06:04
    Worklog Time Spent: 10m 
      Work Description: kinow commented on a change in pull request #165:
URL: https://github.com/apache/commons-imaging/pull/165#discussion_r704981060



##########
File path: 
src/main/java/org/apache/commons/imaging/formats/tiff/TiffImageParser.java
##########
@@ -762,12 +763,12 @@ public void writeImage(final BufferedImage src, final 
OutputStream os, final Map
     }
 
     /**
-     * Reads the content of a TIFF file that contains floating-point data
-     * samples.
+     * Reads the content of a TIFF file that contains numerical data samples
+     * rather than image-related pixels.
      * <p>
-     * If desired, sub-image data can be read from the file by using a Java Map
-     * instance to specify the subsection of the image that is required. The
-     * following code illustrates the approach:
+     * If desired, sub-image data can be read from the file by using a Java
+     * {@code Map} instance to specify the subsection of the image that
+     * isrequired. The following code illustrates the approach:

Review comment:
       s/isrequired/is required?

##########
File path: 
src/test/java/org/apache/commons/imaging/formats/tiff/TiffShortIntRoundTripTest.java
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.awt.image.BufferedImage;

Review comment:
       Not used.

##########
File path: 
src/test/java/org/apache/commons/imaging/formats/tiff/TiffRasterDataIntTest.java
##########
@@ -0,0 +1,203 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Provides unit test for the raster-data class.
+ */
+public class TiffRasterDataIntTest {
+
+    int width = 11;
+    int height = 10;
+    int[] data;
+    TiffRasterData raster;
+    float meanValue;
+
+    public TiffRasterDataIntTest() {
+        double sum = 0;
+        data = new int[width * height];
+        int k = 0;
+        for (int i = 0; i < width; i++) {
+            for (int j = 0; j < height; j++) {
+                data[k] = (int)k;

Review comment:
       Unnecessary casting.

##########
File path: 
src/test/java/org/apache/commons/imaging/formats/tiff/TiffRasterDataIntTest.java
##########
@@ -0,0 +1,203 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Provides unit test for the raster-data class.
+ */
+public class TiffRasterDataIntTest {
+
+    int width = 11;
+    int height = 10;
+    int[] data;
+    TiffRasterData raster;
+    float meanValue;
+
+    public TiffRasterDataIntTest() {
+        double sum = 0;
+        data = new int[width * height];
+        int k = 0;
+        for (int i = 0; i < width; i++) {
+            for (int j = 0; j < height; j++) {
+                data[k] = (int)k;
+                sum += k;
+                k++;
+            }
+        }
+        raster = new TiffRasterDataInt(width, height, data);
+        meanValue = (float) (sum / k);
+    }
+
+    /**
+     * Test of setValue method, of class TiffRasterData.
+     */
+    @Test
+    public void testSetValue() {
+        final TiffRasterData instance = new TiffRasterDataInt(width, height);
+        for (int y = 0; y < height; y++) {
+            for (int x = 0; x < width; x++) {
+                final int index = y * width + height;
+                instance.setValue(x, y, index+0.4f);
+                int test = (int) instance.getValue(x, y);
+                assertEquals(index, test, "Set/get value test failed");
+                               instance.setIntValue(x, y, index);
+                test = (int) instance.getIntValue(x, y);

Review comment:
       Unnecessary casting.

##########
File path: 
src/test/java/org/apache/commons/imaging/formats/tiff/TiffRasterDataIntTest.java
##########
@@ -0,0 +1,203 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Provides unit test for the raster-data class.
+ */
+public class TiffRasterDataIntTest {
+
+    int width = 11;
+    int height = 10;
+    int[] data;
+    TiffRasterData raster;
+    float meanValue;
+
+    public TiffRasterDataIntTest() {
+        double sum = 0;
+        data = new int[width * height];
+        int k = 0;
+        for (int i = 0; i < width; i++) {
+            for (int j = 0; j < height; j++) {
+                data[k] = (int)k;
+                sum += k;
+                k++;
+            }
+        }
+        raster = new TiffRasterDataInt(width, height, data);
+        meanValue = (float) (sum / k);
+    }
+
+    /**
+     * Test of setValue method, of class TiffRasterData.
+     */
+    @Test
+    public void testSetValue() {
+        final TiffRasterData instance = new TiffRasterDataInt(width, height);
+        for (int y = 0; y < height; y++) {
+            for (int x = 0; x < width; x++) {
+                final int index = y * width + height;
+                instance.setValue(x, y, index+0.4f);
+                int test = (int) instance.getValue(x, y);
+                assertEquals(index, test, "Set/get value test failed");
+                               instance.setIntValue(x, y, index);
+                test = (int) instance.getIntValue(x, y);
+                assertEquals(index, test, "Set/get int value test failed");
+            }
+        }
+    }
+
+    /**
+     * Test of getValue method, of class TiffRasterData.
+     */
+    @Test
+    public void testGetValue() {
+        for (int y = 0; y < height; y++) {
+            for (int x = 0; x < width; x++) {
+                final int index = y * width + x;
+                int test = (int) raster.getValue(x, y);
+                assertEquals(index, test, "Get into source data test failed at 
(" + x + "," + y + ")");
+                test = (int) raster.getIntValue(x, y);

Review comment:
       Unnecessary casting.

##########
File path: 
src/test/java/org/apache/commons/imaging/formats/tiff/TiffRasterDataIntTest.java
##########
@@ -0,0 +1,203 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Provides unit test for the raster-data class.
+ */
+public class TiffRasterDataIntTest {
+
+    int width = 11;
+    int height = 10;
+    int[] data;
+    TiffRasterData raster;
+    float meanValue;
+
+    public TiffRasterDataIntTest() {
+        double sum = 0;
+        data = new int[width * height];
+        int k = 0;
+        for (int i = 0; i < width; i++) {
+            for (int j = 0; j < height; j++) {
+                data[k] = (int)k;
+                sum += k;
+                k++;
+            }
+        }
+        raster = new TiffRasterDataInt(width, height, data);
+        meanValue = (float) (sum / k);
+    }
+
+    /**
+     * Test of setValue method, of class TiffRasterData.
+     */
+    @Test
+    public void testSetValue() {
+        final TiffRasterData instance = new TiffRasterDataInt(width, height);
+        for (int y = 0; y < height; y++) {
+            for (int x = 0; x < width; x++) {
+                final int index = y * width + height;
+                instance.setValue(x, y, index+0.4f);
+                int test = (int) instance.getValue(x, y);
+                assertEquals(index, test, "Set/get value test failed");
+                               instance.setIntValue(x, y, index);
+                test = (int) instance.getIntValue(x, y);
+                assertEquals(index, test, "Set/get int value test failed");
+            }
+        }
+    }
+
+    /**
+     * Test of getValue method, of class TiffRasterData.
+     */
+    @Test
+    public void testGetValue() {
+        for (int y = 0; y < height; y++) {
+            for (int x = 0; x < width; x++) {
+                final int index = y * width + x;
+                int test = (int) raster.getValue(x, y);
+                assertEquals(index, test, "Get into source data test failed at 
(" + x + "," + y + ")");
+                test = (int) raster.getIntValue(x, y);
+                assertEquals(index, test, "Get into source data test failed at 
(" + x + "," + y + ")");
+            }
+        }
+    }
+
+    /**
+     * Test of getSimpleStatistics method, of class TiffRasterData.
+     */
+    @Test
+    public void testGetSimpleStatistics_0args() {
+
+        final TiffRasterStatistics result = raster.getSimpleStatistics();
+        assertEquals(0, result.getMinValue(), "Min value failure");
+        assertEquals(width * height - 1, result.getMaxValue(), "Max value 
failure");
+        assertEquals(meanValue, result.getMeanValue(), "Mean value failure");
+    }
+
+    /**
+     * Test of getSimpleStatistics method, of class TiffRasterData.
+     */
+    @Test
+    public void testGetSimpleStatistics_float() {
+        // exclude the maximum value (width*height-1).  This will result
+        // in a max value of width*height-2
+        final TiffRasterStatistics result = raster.getSimpleStatistics(width * 
height - 1);
+        assertEquals(width * height - 2, result.getMaxValue(), "Max value 
failure");
+    }
+
+    /**
+     * Test of getWidth method, of class TiffRasterData.
+     */
+    @Test
+    public void testGetWidth() {
+        assertEquals(width, raster.getWidth(), "Improper width stored");
+    }
+
+    /**
+     * Test of getHeight method, of class TiffRasterData.
+     */
+    @Test
+    public void testGetHeight() {
+        assertEquals(width, raster.getWidth(), "Improper height stored");
+    }
+
+    /**
+     * Test of getData method, of class TiffRasterData.
+     */
+    @Test
+    public void testGetData() {
+        final float[] result = raster.getData();
+               for(int i=0; i<result.length; i++){
+                       assertEquals((int)result[i], data[i]);
+               }
+               final int []iResult = raster.getIntData();
+        assertArrayEquals(data, iResult);
+    }
+
+
+     /**
+     * Test of constructors with bad arguments, of class TiffRasterData.
+     */
+    @Test
+    public void testBadConstructor() {
+        try{
+            final TiffRasterData raster = new TiffRasterDataInt(-1, 10);
+            fail("Constructor did not detect bad width");
+        }catch(final IllegalArgumentException illArgEx){
+            // success!
+        }

Review comment:
       @gwlucastrig I believe the tests that are using this pattern `try { 
something; fail(); } catch {}` is JUnit 4. But for Junit 5 we can use:
   
   ```java
   assertThrows(IllegalArgumentException.class, () -> new TiffRasterDataInt(-1, 
10));
   ``` 
   
   There are other places in this PR where this can be applied, you can look at 
GitHub's UI to search for this "fail(" text if you'd like.

##########
File path: 
src/test/java/org/apache/commons/imaging/formats/tiff/TiffRasterDataIntTest.java
##########
@@ -0,0 +1,203 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Provides unit test for the raster-data class.
+ */
+public class TiffRasterDataIntTest {
+
+    int width = 11;
+    int height = 10;
+    int[] data;
+    TiffRasterData raster;
+    float meanValue;
+
+    public TiffRasterDataIntTest() {
+        double sum = 0;
+        data = new int[width * height];
+        int k = 0;
+        for (int i = 0; i < width; i++) {
+            for (int j = 0; j < height; j++) {
+                data[k] = (int)k;
+                sum += k;
+                k++;
+            }
+        }
+        raster = new TiffRasterDataInt(width, height, data);
+        meanValue = (float) (sum / k);
+    }
+
+    /**
+     * Test of setValue method, of class TiffRasterData.
+     */
+    @Test
+    public void testSetValue() {
+        final TiffRasterData instance = new TiffRasterDataInt(width, height);
+        for (int y = 0; y < height; y++) {
+            for (int x = 0; x < width; x++) {
+                final int index = y * width + height;
+                instance.setValue(x, y, index+0.4f);
+                int test = (int) instance.getValue(x, y);
+                assertEquals(index, test, "Set/get value test failed");
+                               instance.setIntValue(x, y, index);
+                test = (int) instance.getIntValue(x, y);
+                assertEquals(index, test, "Set/get int value test failed");
+            }
+        }
+    }
+
+    /**
+     * Test of getValue method, of class TiffRasterData.
+     */
+    @Test
+    public void testGetValue() {
+        for (int y = 0; y < height; y++) {
+            for (int x = 0; x < width; x++) {
+                final int index = y * width + x;
+                int test = (int) raster.getValue(x, y);
+                assertEquals(index, test, "Get into source data test failed at 
(" + x + "," + y + ")");
+                test = (int) raster.getIntValue(x, y);
+                assertEquals(index, test, "Get into source data test failed at 
(" + x + "," + y + ")");
+            }
+        }
+    }
+
+    /**
+     * Test of getSimpleStatistics method, of class TiffRasterData.
+     */
+    @Test
+    public void testGetSimpleStatistics_0args() {
+
+        final TiffRasterStatistics result = raster.getSimpleStatistics();
+        assertEquals(0, result.getMinValue(), "Min value failure");
+        assertEquals(width * height - 1, result.getMaxValue(), "Max value 
failure");
+        assertEquals(meanValue, result.getMeanValue(), "Mean value failure");
+    }
+
+    /**
+     * Test of getSimpleStatistics method, of class TiffRasterData.
+     */
+    @Test
+    public void testGetSimpleStatistics_float() {
+        // exclude the maximum value (width*height-1).  This will result
+        // in a max value of width*height-2
+        final TiffRasterStatistics result = raster.getSimpleStatistics(width * 
height - 1);
+        assertEquals(width * height - 2, result.getMaxValue(), "Max value 
failure");
+    }
+
+    /**
+     * Test of getWidth method, of class TiffRasterData.
+     */
+    @Test
+    public void testGetWidth() {
+        assertEquals(width, raster.getWidth(), "Improper width stored");
+    }
+
+    /**
+     * Test of getHeight method, of class TiffRasterData.
+     */
+    @Test
+    public void testGetHeight() {
+        assertEquals(width, raster.getWidth(), "Improper height stored");
+    }
+
+    /**
+     * Test of getData method, of class TiffRasterData.
+     */
+    @Test
+    public void testGetData() {
+        final float[] result = raster.getData();
+               for(int i=0; i<result.length; i++){
+                       assertEquals((int)result[i], data[i]);
+               }
+               final int []iResult = raster.getIntData();
+        assertArrayEquals(data, iResult);

Review comment:
       Tabs and spaces here @gwlucastrig :point_up: (better confirm other files 
don't have tabs, only spotted it here due to the lines not aligned due to mix 
spaces/tabs, but we could have in the other files).




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Issue Time Tracking
-------------------

    Worklog Id:     (was: 648401)
    Time Spent: 40m  (was: 0.5h)

> Read integer data  from GeoTIFFS
> --------------------------------
>
>                 Key: IMAGING-266
>                 URL: https://issues.apache.org/jira/browse/IMAGING-266
>             Project: Commons Imaging
>          Issue Type: New Feature
>          Components: Format: TIFF
>    Affects Versions: 1.0-alpha3
>            Reporter: Gary Lucas
>            Priority: Major
>          Time Spent: 40m
>  Remaining Estimate: 0h
>
> I recently discovered that there is a large amount of digital elevation data 
> available in the form of 16-bit integer coded data in GeoTIFF files (TIFF 
> files with geographic tags).  I propose to enhance the Commons Imaging API to 
> read these files.  This work will be similar to the work I did for reading 
> floating-point raster data under ISSUE-251.
> Available data include the nearly-global coverage of one-second of arc 
> elevation data produced from the Shuttle Radar Topography Mission (SRTM) and 
> other sources. These products give grids of elevation data with a 30 meter 
> cell spacing for most of the world's land masses. They are available at NASA 
> Earthdata and Japan Space Systems websites, see 
> [https://asterweb.jpl.nasa.gov/gdem.asp|https://asterweb.jpl.nasa.gov/gdem.asp]
>  There is also a ocean bathymetry data set available in this format at 
> [http://www.shadedrelief.com/blue-earth/]
> This new feature will continue to expand the usefulness of the Commons 
> Imaging API in accessing GeoTIFF products.
> Request for Feedback
> So far, the data products I've found (ASTER and Blue Earth Bathymetry) give 
> elevation and ocean depth data in meters recorded as a short integer.  I 
> haven't found an example of where the 32-bit integer format is used.  For 
> now, I am planning on only coding the 16-bit integer variation.  Does anyone 
> know if the 32-bit version is worth supporting?  My criteria for determining 
> that would be based on whether there is a significant number of projects 
> using that format (life is too short to chase rarely used data formats).
> Currently, one of the code-analysis operations conducted by the Commons 
> Imaging build process is coverage by JUnit tests.  Lacking any test data for 
> the 32-bit case, I am reluctant to include it in the code base because it 
> would mean putting uncovered code into the distribution.
> Also, I am wondering about the best design for the access API.  The current 
> TiffImageParser class has a method called getFloatingPointRasterData() that 
> returns an instance of TiffRasterData.  TiffRasterData is pretty much 
> hard-wired to floating point data.  I am thinking of creating a new method 
> called getIntegerRasterData() that would return an instance of a new class 
> called TiffIntegerRasterData. Does this seem reasonable?  I considered trying 
> to combine both kinds of results into a unified class and method, but it 
> seems more unwieldy than useful. 
>  
>  



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Reply via email to