Fix silent loss of CF-conformant NetCDF grids

vortex-api/src/main/java/mil/army/usace/hec/vortex/VortexGrid.java

@@ -327,10 +327,13 @@ public float[][][] data3D() {
     private VortexDataType inferDataType() {
         if (interval == null || interval.isZero()) return VortexDataType.INSTANTANEOUS;
 
+        // Only a non-zero interval reaches here, so the record spans a period by definition. Naming it a
+        // point type would make it self-contradictory, and InstantaneousRecordIndexQuery would then drop
+        // every such record from its index. AVERAGE is the safe period type for an unrecognized variable:
+        // ACCUMULATION would imply the values are summable over the interval, which is not knowable here.
         return switch (VortexVariable.fromName(shortName)) {
             case PRECIPITATION -> VortexDataType.ACCUMULATION;
-            case TEMPERATURE, SHORTWAVE_RADIATION, WINDSPEED, PRESSURE -> VortexDataType.AVERAGE;
-            default -> VortexDataType.INSTANTANEOUS;
+            default -> VortexDataType.AVERAGE;
         };
     }
 

vortex-api/src/main/java/mil/army/usace/hec/vortex/io/GridDatasetReader.java

@@ -1,6 +1,7 @@
 package mil.army.usace.hec.vortex.io;
 
 import mil.army.usace.hec.vortex.VortexData;
+import mil.army.usace.hec.vortex.VortexDataType;
 import mil.army.usace.hec.vortex.VortexGrid;
 import mil.army.usace.hec.vortex.geo.*;
 import mil.army.usace.hec.vortex.util.UnitUtil;
@@ -235,7 +236,7 @@ private VortexGrid buildGrid(float[] data, VortexDataInterval timeRecord) {
                 .startTime(timeRecord.startTime())
                 .endTime(timeRecord.endTime())
                 .interval(timeRecord.getRecordDuration())
-                .dataType(getVortexDataType(variableDS))
+                .dataType(getDeclaredDataType())
                 .build();
     }
 
@@ -416,6 +417,16 @@ private VortexDataInterval adjustTimeForSpecialFile(CoordinateAxis1DTime tAxis,
         return VortexDataInterval.of(adjustedStart, adjustedEnd);
     }
 
+    private String getTimeAxisName() {
+        CoordinateAxis timeAxis = gridCoordSystem.getTimeAxis();
+        return timeAxis != null ? timeAxis.getShortName() : null;
+    }
+
+    @Override
+    VortexDataType getDeclaredDataType() {
+        return getVortexDataType(variableDS, getTimeAxisName());
+    }
+
     private boolean isSpecialTimeBounds() {
         return specialFileType != null && specialFileType != UNDEFINED;
     }

vortex-api/src/main/java/mil/army/usace/hec/vortex/io/InstantaneousRecordIndexQuery.java

@@ -91,17 +91,45 @@ private static List<Integer> queryPeriod(NavigableMap<ZonedDateTime, Integer> in
     private static NavigableMap<ZonedDateTime, Integer> initInstantaneousDataTree(List<VortexDataInterval> recordList) {
         TreeMap<ZonedDateTime, Integer> treeMap = new TreeMap<>();
 
+        int undefinedCount = 0;
+        int spanningCount = 0;
+
         for (int i = 0; i < recordList.size(); i++) {
             VortexDataInterval timeRecord = recordList.get(i);
-            boolean isUndefined = !VortexDataInterval.isDefined(timeRecord);
 
-            if (isUndefined || !timeRecord.isInstantaneous()) {
+            if (!VortexDataInterval.isDefined(timeRecord)) {
+                undefinedCount++;
+                continue;
+            }
+
+            if (!timeRecord.isInstantaneous()) {
+                spanningCount++;
                 continue;
             }
 
             treeMap.put(timeRecord.startTime(), i);
         }
 
+        logSkippedRecords(recordList.size(), undefinedCount, spanningCount);
+
         return Collections.unmodifiableNavigableMap(treeMap);
     }
+
+    /**
+     * A record that spans a period cannot be indexed as an instant, so it is dropped. That is a
+     * classification defect — the data was typed INSTANTANEOUS but its start and end times differ — and
+     * dropping every record leaves the reader with no time range at all. Report it rather than let the
+     * caller discover it as an empty read.
+     */
+    private static void logSkippedRecords(int total, int undefinedCount, int spanningCount) {
+        if (spanningCount > 0) {
+            logger.warning(() -> "Skipped " + spanningCount + " of " + total + " instantaneous records "
+                    + "with differing start and end times. Data typed as instantaneous must not span a "
+                    + "period; check the source's cell_methods and time bounds.");
+        }
+
+        if (undefinedCount > 0) {
+            logger.info(() -> "Skipped " + undefinedCount + " of " + total + " instantaneous records with undefined times.");
+        }
+    }
 }

vortex-api/src/main/java/mil/army/usace/hec/vortex/io/NetcdfDataReader.java

@@ -24,12 +24,16 @@
 import java.util.*;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 abstract class NetcdfDataReader extends DataReader {
     private static final Logger logger = Logger.getLogger(NetcdfDataReader.class.getName());
 
     private static final PathMatcher NC_MATCHER = FileSystems.getDefault().getPathMatcher("regex:(?i).*\\.nc4?");
     private static final String TIME_BOUNDS = "time_bnds";
+    private static final Pattern CELL_METHODS_QUALIFIER = Pattern.compile("\\([^)]*\\)");
+    private static final Pattern TIME_CELL_METHOD = compileCellMethodPattern(CF.TIME);
 
     /* Factory Method */
     public static NetcdfDataReader createInstance(String pathToFile, String pathToData) throws DataReadException {
@@ -154,28 +158,98 @@ static void shiftGrid(Grid grid) {
         }
     }
 
-    static VortexDataType getVortexDataType(VariableDS variableDS) {
+    /**
+     * Resolves a variable's data type from its CF {@code cell_methods} attribute.
+     *
+     * @param timeAxisName the short name of the time coordinate variable, or null when it is unknown.
+     *                     CF allows a cell_methods entry to be keyed on the time coordinate variable's
+     *                     own name rather than the literal "time"; both are accepted.
+     */
+    static VortexDataType getVortexDataType(VariableDS variableDS, String timeAxisName) {
         String cellMethods = variableDS.findAttributeString(CF.CELL_METHODS, "");
-        return VortexDataType.fromString(cellMethods);
+        return VortexDataType.fromString(parseTimeCellMethod(cellMethods, timeAxisName));
+    }
+
+    /**
+     * Extracts the method applied to the time coordinate from a CF cell_methods string. CF-1.11 §7.3
+     * defines the attribute as a blank-separated list of "name: method [(qualifiers)]" entries, so the
+     * method has to be pulled out before it can be mapped to a {@link VortexDataType}. A string with no
+     * "name:" entry is returned unchanged, so the bare tokens written by {@link NetcdfWriterPrep}
+     * before it emitted CF-conformant output ("mean", "sum", "point") keep resolving as they always have.
+     *
+     * @return the method applied to the time coordinate, or an empty string when the attribute declares
+     * no method for it (a cell_methods of "area: mean" says nothing about how time was reduced).
+     */
+    static String parseTimeCellMethod(String cellMethods, String timeAxisName) {
+        if (cellMethods == null || cellMethods.isBlank()) return "";
+
+        // Qualifiers are dropped first: "(interval: 1 day)" contains a colon of its own.
+        String stripped = CELL_METHODS_QUALIFIER.matcher(cellMethods).replaceAll(" ");
+        if (!stripped.contains(":")) return stripped.trim();
+
+        Matcher matcher = timeCellMethodPattern(timeAxisName).matcher(stripped);
+        return matcher.find() ? matcher.group(1) : "";
+    }
+
+    private static Pattern timeCellMethodPattern(String timeAxisName) {
+        boolean isDefaultName = timeAxisName == null || timeAxisName.isBlank() || timeAxisName.equalsIgnoreCase(CF.TIME);
+        if (isDefaultName) return TIME_CELL_METHOD;
+        return compileCellMethodPattern(CF.TIME + "|" + Pattern.quote(timeAxisName));
+    }
+
+    private static Pattern compileCellMethodPattern(String names) {
+        return Pattern.compile("(?:^|\\s)(?:" + names + ")\\s*:\\s*([A-Za-z_]+)", Pattern.CASE_INSENSITIVE);
     }
 
     @Override
     public Validation isValid() {
+        List<String> messages = new ArrayList<>();
+
         try (NetcdfDataset dataset = NetcdfDatasets.openDataset(path)) {
             Path pathToFile = Path.of(path);
             if (NC_MATCHER.matches(pathToFile)) {
                 Variable variable = dataset.findVariable(TIME_BOUNDS);
                 if (variable == null) {
-                    String message = Message.format("warn_nc_time_bnds");
-                    return Validation.of(true, message);
+                    messages.add(Message.format("warn_nc_time_bnds"));
                 }
             }
         } catch (IOException e) {
             String message = Message.format("error_invalid_file", path);
             return Validation.of(false, message);
         }
-        return Validation.of(true);
+
+        if (hasSpanningInstantaneousRecords()) {
+            messages.add(Message.format("warn_nc_instantaneous_span", variableName));
+        }
+
+        return messages.isEmpty() ? Validation.of(true) : Validation.of(true, messages);
+    }
+
+    /**
+     * Reports whether the variable declares itself instantaneous while its time bounds span a period.
+     * Such records cannot be indexed as instants and are dropped, which leaves the reader with no time
+     * range and every read empty. Without this check the condition is invisible until compute time.
+     */
+    private boolean hasSpanningInstantaneousRecords() {
+        if (getDeclaredDataType() != VortexDataType.INSTANTANEOUS) {
+            return false;
+        }
+
+        try {
+            return getDataIntervals().stream()
+                    .filter(VortexDataInterval::isDefined)
+                    .anyMatch(interval -> !interval.isInstantaneous());
+        } catch (DataReadException e) {
+            logger.log(Level.INFO, e, e::getMessage);
+            return false;
+        }
     }
 
+    /**
+     * The data type declared by the source variable's CF cell_methods attribute, before any inference
+     * {@link mil.army.usace.hec.vortex.VortexGrid#dataType()} applies on top of it.
+     */
+    abstract VortexDataType getDeclaredDataType();
+
     abstract double getNoDataValue();
 }

vortex-api/src/main/java/mil/army/usace/hec/vortex/io/NetcdfWriterPrep.java

@@ -263,11 +263,22 @@ private static void addVariableGridCollection(NetcdfFormatWriter.Builder writerB
                     .addAttribute(new Attribute(CF.COORDINATES, "latitude longitude"))
                     .addAttribute(new Attribute(CF.MISSING_VALUE, (float) vortexGrid.noDataValue()))
                     .addAttribute(new Attribute(CF._FILLVALUE, (float) vortexGrid.noDataValue()))
-                    .addAttribute(new Attribute(CF.CELL_METHODS, vortexGrid.dataType().getNcString()));
+                    .addAttribute(new Attribute(CF.CELL_METHODS, getCellMethods(vortexGrid)));
         }
 
     }
 
+    /**
+     * Builds the CF cell_methods attribute for a grid. CF-1.11 §7.3 requires each entry to name the
+     * dimension the method was applied to, so the method is keyed on the time dimension this writer
+     * creates. Earlier versions wrote the bare method ("mean", "sum", "point"); NetcdfDataReader still
+     * reads that form, so files written before this change keep resolving to the same data type.
+     */
+    private static String getCellMethods(VortexGrid vortexGrid) {
+        String method = vortexGrid.dataType().getNcString();
+        return method.isBlank() ? method : CF.TIME + ": " + method;
+    }
+
     private static void addGlobalAttributes(NetcdfFormatWriter.Builder writerBuilder) {
         writerBuilder.addAttribute(new Attribute("Conventions", "CF-1.10"));
     }

vortex-api/src/main/java/mil/army/usace/hec/vortex/io/VariableDsReader.java

@@ -1,6 +1,7 @@
 package mil.army.usace.hec.vortex.io;
 
 import mil.army.usace.hec.vortex.VortexData;
+import mil.army.usace.hec.vortex.VortexDataType;
 import mil.army.usace.hec.vortex.VortexGrid;
 import mil.army.usace.hec.vortex.geo.Grid;
 import mil.army.usace.hec.vortex.geo.ReferenceUtils;
@@ -148,6 +149,16 @@ private CoordinateAxis1D getTimeAxis() {
         return timeAxis instanceof CoordinateAxis1D axis ? axis : null;
     }
 
+    private String getTimeAxisName() {
+        CoordinateAxis1D timeAxis = getTimeAxis();
+        return timeAxis != null ? timeAxis.getShortName() : null;
+    }
+
+    @Override
+    VortexDataType getDeclaredDataType() {
+        return getVortexDataType(variableDS, getTimeAxisName());
+    }
+
     private List<VortexDataInterval> getYearMonthTimeRecords(CoordinateAxis1D timeAxis) {
         List<VortexDataInterval> timeRecords = new ArrayList<>();
         for (int i = 0; i < getDtoCount(); i++) {
@@ -249,7 +260,7 @@ private VortexGrid buildGrid(float[] data, VortexDataInterval timeRecord) {
                 .startTime(timeRecord.startTime())
                 .endTime(timeRecord.endTime())
                 .interval(timeRecord.getRecordDuration())
-                .dataType(getVortexDataType(variableDS))
+                .dataType(getDeclaredDataType())
                 .build();
     }
 

vortex-api/src/main/resources/message.properties

@@ -61,6 +61,7 @@ time_shifter_time=Elapsed time: {0}.
 error_invalid_file=File "\{0}\" could not be opened.
 # NetcdfDataReader
 warn_nc_time_bnds=CF compliance check failed: One or more NetCDF datasets do not contain a "time_bnds" variable. Imported start/end times may be inaccurate. Use the time-shifter utility to shift start and/or end times after import. \n\nDo you want to proceed?
+warn_nc_instantaneous_span=CF compliance check failed: Variable "{0}" declares a "cell_methods" of "point" but its time bounds span a period. Records that span a period cannot be read as instantaneous values and will be skipped. \n\nDo you want to proceed?
 # ImportMetWizard
 error_archive_file=Unrecognized archive format for file: \"{0}\".
 error_archive_file_suggestion=Try extracting the contents of the archive before import.

vortex-api/src/test/java/mil/army/usace/hec/vortex/io/NetcdfDataReaderTest.java

@@ -24,6 +24,7 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.time.Instant;
 import java.time.ZoneOffset;
 import java.time.ZonedDateTime;
 import java.util.*;
@@ -1074,4 +1075,77 @@ void gpmHalfHourV07C_intermediatePrecipFields() throws Exception {
             assertEquals("PER-CUM", grid.dataType().getDssString(), variable);
         }
     }
+
+    @Test
+    void parseTimeCellMethodReadsCfSyntax() {
+        // CF-1.11 section 7.3: a blank-separated list of "name: method [(qualifiers)]" entries.
+        assertEquals("mean", NetcdfDataReader.parseTimeCellMethod("time: mean", null));
+        assertEquals("sum", NetcdfDataReader.parseTimeCellMethod("time: sum", null));
+        assertEquals("point", NetcdfDataReader.parseTimeCellMethod("time: point", null));
+
+        // The time entry is found wherever it appears in the list.
+        assertEquals("maximum", NetcdfDataReader.parseTimeCellMethod("area: mean time: maximum", null));
+
+        // Qualifiers are dropped, including the colon inside them.
+        assertEquals("mean", NetcdfDataReader.parseTimeCellMethod("time: mean (interval: 1 day)", null));
+
+        // Climatological statistics list several time entries; the first method wins.
+        assertEquals("mean", NetcdfDataReader.parseTimeCellMethod("time: mean within days time: mean over days", null));
+
+        // Nothing was declared about time, so nothing is claimed about it.
+        assertEquals("", NetcdfDataReader.parseTimeCellMethod("area: mean", null));
+
+        // Bare tokens, the form vortex itself wrote before it emitted CF-conformant output.
+        assertEquals("mean", NetcdfDataReader.parseTimeCellMethod("mean", null));
+        assertEquals("sum", NetcdfDataReader.parseTimeCellMethod("sum", null));
+        assertEquals("point", NetcdfDataReader.parseTimeCellMethod("point", null));
+
+        // Absent or empty attribute.
+        assertEquals("", NetcdfDataReader.parseTimeCellMethod("", null));
+        assertEquals("", NetcdfDataReader.parseTimeCellMethod("   ", null));
+        assertEquals("", NetcdfDataReader.parseTimeCellMethod(null, null));
+
+        // CF permits the entry to name the time coordinate variable instead of the literal "time".
+        assertEquals("mean", NetcdfDataReader.parseTimeCellMethod("valid_time: mean", "valid_time"));
+        assertEquals("", NetcdfDataReader.parseTimeCellMethod("valid_time: mean", null));
+        assertEquals("mean", NetcdfDataReader.parseTimeCellMethod("time: mean", "valid_time"));
+    }
+
+    @Test
+    void parseTimeCellMethodMapsToDataType() {
+        assertEquals(VortexDataType.AVERAGE, VortexDataType.fromString(NetcdfDataReader.parseTimeCellMethod("time: mean", null)));
+        assertEquals(VortexDataType.ACCUMULATION, VortexDataType.fromString(NetcdfDataReader.parseTimeCellMethod("time: sum", null)));
+        assertEquals(VortexDataType.INSTANTANEOUS, VortexDataType.fromString(NetcdfDataReader.parseTimeCellMethod("time: point", null)));
+        assertEquals(VortexDataType.UNDEFINED, VortexDataType.fromString(NetcdfDataReader.parseTimeCellMethod("area: mean", null)));
+        assertEquals(VortexDataType.AVERAGE, VortexDataType.fromString(NetcdfDataReader.parseTimeCellMethod("mean", null)));
+    }
+
+    /**
+     * cf_style.nc and bare_mean.nc are identical but for the cell_methods attribute: the first declares
+     * CF's "time: mean", the second the bare "mean" that vortex used to write. Both must classify the
+     * same, and their records must be reachable through TemporalDataReader.
+     */
+    @Test
+    void cellMethodsVariantsReadTheSame() throws Exception {
+        for (String resource : List.of("/cf_style.nc", "/bare_mean.nc")) {
+            String file = new File(Objects.requireNonNull(getClass().getResource(resource)).getFile()).toString();
+
+            try (DataReader reader = DataReader.builder().path(file).variable("SWE_Post").build()) {
+                VortexGrid grid = (VortexGrid) reader.getDtos().get(0);
+                assertEquals(VortexDataType.AVERAGE, grid.dataType(), resource);
+
+                // Each step spans a day, per time_bnds.
+                assertEquals(Instant.parse("2002-10-01T00:00:00Z"), grid.startTime().toInstant(), resource);
+                assertEquals(Instant.parse("2002-10-02T00:00:00Z"), grid.endTime().toInstant(), resource);
+
+                TemporalDataReader temporal = TemporalDataReader.create(reader);
+                assertEquals(Instant.parse("2002-10-01T00:00:00Z"),
+                        temporal.getStartTime().orElseThrow().toInstant(), resource);
+                // A period type reports the end of the last interval, not the last timestamp.
+                assertEquals(Instant.parse("2002-10-04T00:00:00Z"),
+                        temporal.getEndTime().orElseThrow().toInstant(), resource);
+                assertTrue(temporal.readNearest(ZonedDateTime.parse("2002-10-01T12:00Z")).isPresent(), resource);
+            }
+        }
+    }
 }