search

Unidata / netcdf-java

The Unidata netcdf-java library
https://docs.unidata.ucar.edu/netcdf-java/current/userguide/index.html
BSD 3-Clause "New" or "Revised" License
145 stars 70 forks source link

dap4.test.TestNc4Iosp failure on Jenkins #126

Open lesserwhirls opened 5 years ago

lesserwhirls commented 5 years ago

We have a new failure on Jenkins for dap4.test.TestNc4Iosp.testNc4Iosp. This started showing up when the way we load IOSPs changed with PR 101 (see https://github.com/Unidata/netcdf-java/pull/101/files for the changes).

My suspicion is that the change caused the dap4 library to use the Hdf5Iosp instead of the Nc4Iosp. The code should handle both cases, but I think what we see is that there is a difference in the way the way the two IOSPs see variable metadata (in this case, something about the way enum is handled). Here is the output from Jenkins:

Netcdf-c library version: 4.6.1 of Mar 30 2018 02:30:19 $
Testcase: /home/ubuntu/jenkins/workspace/netcdf-java/dap4/d4tests/src/test/data/resources/testfiles/test_one_var.nc
Testpath: /home/ubuntu/jenkins/workspace/netcdf-java/dap4/d4tests/src/test/data/resources/testfiles/test_one_var.nc
Baseline: /home/ubuntu/jenkins/workspace/netcdf-java/dap4/d4tests/src/test/data/resources/TestIosp/baseline/test_one_var.nc.nc4
DMR Comparison:
Files are Identical
DATA Comparison:
Files are Identical
Testcase: /home/ubuntu/jenkins/workspace/netcdf-java/dap4/d4tests/src/test/data/resources/testfiles/test_one_vararray.nc
Testpath: /home/ubuntu/jenkins/workspace/netcdf-java/dap4/d4tests/src/test/data/resources/testfiles/test_one_vararray.nc
Baseline: /home/ubuntu/jenkins/workspace/netcdf-java/dap4/d4tests/src/test/data/resources/TestIosp/baseline/test_one_vararray.nc.nc4
DMR Comparison:
Files are Identical
DATA Comparison:
Files are Identical
Testcase: /home/ubuntu/jenkins/workspace/netcdf-java/dap4/d4tests/src/test/data/resources/testfiles/test_atomic_types.nc
Testpath: /home/ubuntu/jenkins/workspace/netcdf-java/dap4/d4tests/src/test/data/resources/testfiles/test_atomic_types.nc
Baseline: /home/ubuntu/jenkins/workspace/netcdf-java/dap4/d4tests/src/test/data/resources/TestIosp/baseline/test_atomic_types.nc.nc4
DMR Comparison:
>>>> 18 CHANGED FROM
    enum cloud_class_t primary_cloud;
>>>>     CHANGED TO
    enum primary_cloud primary_cloud;
>>>> 20 CHANGED FROM
    enum cloud_class_t secondary_cloud;
>>>>     CHANGED TO
    enum secondary_cloud secondary_cloud;
>>>> Dap4 Testing: End of differences.
DennisHeimbigner commented 5 years ago

This appears to be an example of the problem that only dap4 properly builds CDM enum types. All the other case (including hdf5) do not properly use the enumeration type name.(hence the cloud_class_t). This is another example where the handling of netcdf4/hdf5 tranlation to CDM is incomplete.

JohnLCaron commented 4 years ago

It appears that variables primary_cloud and secondary_cloud should be using typedef cloud_class_t instead of creating their own typedefs.

Current CDM ncdump on ~netcdf-java/dap4/d4tests/src/test/data/resources/testfiles/test_atomic_types.nc:

netcdf c:/dev/github/netcdf-java/dap4/d4tests/src/test/data/resources/testfiles/test_atomic_types.nc {
  types:
    byte enum cloud_class_t { 'Clear' = 0, 'Cumulonimbus' = 1, 'Stratus' = 2, 'Stratocumulus' = 3, 'Cumulus' = 4, 'Altostratus' = 5, 'Nimbostratus' = 6, 'Altocumulus' = 7, 'Cirrostratus' = 8, 'Cirrocumulus' = 9, 'Cirrus' = 10, 'Missing' = 127};
    enum primary_cloud { 'Clear' = 0, 'Cumulonimbus' = 1, 'Stratus' = 2, 'Stratocumulus' = 3, 'Cumulus' = 4, 'Altostratus' = 5, 'Nimbostratus' = 6, 'Altocumulus' = 7, 'Cirrostratus' = 8, 'Cirrocumulus' = 9, 'Cirrus' = 10, 'Missing' = 127};
    enum secondary_cloud { 'Clear' = 0, 'Cumulonimbus' = 1, 'Stratus' = 2, 'Stratocumulus' = 3, 'Cumulus' = 4, 'Altostratus' = 5, 'Nimbostratus' = 6, 'Altocumulus' = 7, 'Cirrostratus' = 8, 'Cirrocumulus' = 9, 'Cirrus' = 10, 'Missing' = 127};

  variables:
    byte v8;

    ubyte vu8;

    short v16;

    ushort vu16;

    int v32;

    uint vu32;

    long v64;

    ulong vu64;

    float vf;

    double vd;

    char vc;

    String vs;

    opaque vo;

    enum primary_cloud primary_cloud;
      :_FillValue = "Missing";

    enum secondary_cloud secondary_cloud;
      :_FillValue = "Missing";

  // global attributes:
}
JohnLCaron commented 4 years ago

At the moment Im not seeing the error. The Data Object "primary_cloud" has a Datatype Message inside of it that duplicates cloud_class_t, complete with duplicated enumValue/enumName map . I dont (yet) see any reference to cloud_class_t that indicates it should use that. So Ive either missed it, or theres some conventions eg for searching for duplicate Datatype Message that is undocumented or I didnt notice. Its puzzling why hdf5 would duplicate the value/name map.

The c library ncdump for this file is (dap4/d4tests/src/test/data/resources/testfiles/test_atomic_types.ncdump):

netcdf test_atomic_types {
types:
  opaque(8) o_t ;
  byte enum cloud_class_t {Clear = 0, Cumulonimbus = 1, Stratus = 2, 
      Stratocumulus = 3, Cumulus = 4, Altostratus = 5, Nimbostratus = 6, 
      Altocumulus = 7, Cirrostratus = 8, Cirrocumulus = 9, Cirrus = 10, 
      Missing = 127} ;
variables:
    byte v8 ;
    ubyte vu8 ;
    short v16 ;
    ushort vu16 ;
    int v32 ;
    uint vu32 ;
    int64 v64 ;
    uint64 vu64 ;
    float vf ;
    double vd ;
    char vc ;
    string vs ;
    o_t vo ;
    cloud_class_t primary_cloud ;
        cloud_class_t primary_cloud:_FillValue = Missing ;
    cloud_class_t secondary_cloud ;
        cloud_class_t secondary_cloud:_FillValue = Missing ;
...
JohnLCaron commented 4 years ago

Note

  opaque(8) o_t ;
  o_t vo ;

which cdm doesnt reproduce:

    opaque vo;

I think that was a conscious decision on my part to have opaque types be anonymous in the CDM (using the KISS principle). The netcdf4 implementors ignored the CDM data model, which, ahem, predated netcdf4 on hdf5, and simply followed the hdf5 data model.

This and other discrepencies are probably the source of Dennis' comment "the handling of netcdf4/hdf5 translation to CDM is incomplete". I think I ignored the changes they made, if they didnt seem critical, at the cost that the ncdumps would differ. Whereas Dennis (probably rightly so) is testing that they are the same.

So, maybe should be reevaluated at this point, or maybe we can live with it, with Dennis' tests always using the jni interface to nectdf4.

DennisHeimbigner commented 4 years ago

Correct. The CDM and netcdf-4 models differ in many respects (mostly having to do with user-defined types). IMO we should use netcdf-4 model. The reason you see those differences is because DAP4 attempts to more closely map to the netcdf-4 model to, among other things,more closely mimic the netcdf-c library DAP4 mapping.

JohnLCaron commented 4 years ago

Do you have any insight into why the enum variables in test_atomic_types.nc duplicate the typedef ( including the value/name maps) inside of itself, instead of referencing a common datatype?

JohnLCaron commented 4 years ago

I see the same thing with 

  opaque(8) o_t ;
  o_t vo ;

That is, the variable vo has its own data type, and im not seeing the reference to the stand alone type.

OTOH I see hdf5 sample files using enums that dont have a stand-alone DataTypeMessage.

So I think the general question may be "how does netcdf4 implement user defined type? Where does a variable using one have a reference to it? Is this documented anywhere?"

lesserwhirls commented 4 years ago

Correct. The CDM and netcdf-4 models differ in many respects (mostly having to do with user-defined types). IMO we should use netcdf-4 model.

In my opinion, I think we should properly map the CDM to the data model that's expected based on usage of netCDF-Java, as long as properly is well defined. Is it the case that the CDM objects lack the necessary data and metadata to compare with what the C library provides?

So I think the general question may be "how does netcdf4 implement user defined type? Where does a variable using one have a reference to it? Is this documented anywhere?"

If it's not documented, then that's a problem with the spec in general that would only be revealed by having multiple implementations. That's one reason why I resist using the C library to do everything HDF/netCDF4 I/O related.

JohnLCaron commented 4 years ago

Added TestEnumTypedef disabled until we have a fix so Travis doesnt fail.

JohnLCaron commented 4 years ago

Is it the case that the CDM objects lack the necessary data and metadata to compare with what the C library provides?

CDM has the correct semantics, but it doesnt implement user-defined types. Instead of "factoring out" the type of a variable to a standalone type, it duplicates the type information in the variable. Just to keep things confusing, so does the internal representation in hdf5 (at least with the example files Im looking at). There must be a way to find the reference to the standalone type, since thats what the netcdf4 library does. However, the CDM data model (aka CDMDM ;^) doesnt have user defined types. For reading, this only shows up in ncdump output.

For EnumTypedefs, theres no reason not to detect the stand alone type and use it if it exists. As you see in the example above, one only needs cloud_class_t, not 3 separate identical ones. So I consider that a bug to fix.

The question of adding user defined types in the CDM is TBD. Would be good to accumulate real-world examples. Also, are they CF compliant?

JohnLCaron commented 4 years ago

I agree that having an independent implementation for reading HDF5 is important and Im willing for now to keep the Java code updated and fix bugs as a Service to Humanity.

As long as theres an easy way for a user to switch between the Java and JNA implementations, I thinks its a win. Is there a command line argument for controlling which iosp is used?

lesserwhirls commented 4 years ago

Thanks for the clarification.

I agree that having an independent implementation for reading HDF5 is important and Im willing for now to keep the Java code updated and fix bugs as a Service to Humanity.

You’re a data format saint. 😇 I’ll follow closely and learn all that I can.

As long as theres an easy way for a user to switch between the Java and JNA implementations, I thinks its a win. Is there a command line argument for controlling which iosp is used?

Not currently, but could certainly create one.

Bigger question - what’s the right level to insert the JNA? Currently we use JNA for reading/writing netCDF-4 through netCDF-C. We could add a new JNA based HDF5 IOSP and call into HDF C? Or, the new IOSP could use the java wrappers produced by the HDF5 build and skip a directly based JNA IOSP.

The question of adding user defined types in the CDM is TBD. Would be good to accumulate real-world examples. Also, are they CF compliant?

According to the list of data types supported by next version CF (1.8) and the guidance here, I would say that in general, no.

JohnLCaron commented 4 years ago

Bigger question - what’s the right level to insert the JNA? Currently we use JNA for reading/writing netCDF-4 through netCDF-C. We could add a new JNA based HDF5 IOSP and call into HDF C? Or, the new IOSP could use the java wrappers produced by the HDF5 build and skip a directly based JNA IOSP.

Can the netcdf-c library read any hdf5 or just netcdf4 ?

On Thu, Jan 2, 2020 at 11:37 AM Sean Arms notifications@github.com wrote:

Thanks for the clarification.

I agree that having an independent implementation for reading HDF5 is important and Im willing for now to keep the Java code updated and fix bugs as a Service to Humanity.

You’re a data format saint. 😇 I’ll follow closely and learn all that I can.

As long as theres an easy way for a user to switch between the Java and JNA implementations, I thinks its a win. Is there a command line argument for controlling which iosp is used?

Not currently, but could certainly create one.

Bigger question - what’s the right level to insert the JNA? Currently we use JNA for reading/writing netCDF-4 through netCDF-C. We could add a new JNA based HDF5 IOSP and call into HDF C? Or, the new IOSP could use the java wrappers produced by the HDF5 build and skip a directly based JNA IOSP.

The question of adding user defined types in the CDM is TBD. Would be good to accumulate real-world examples. Also, are they CF compliant?

According to the list of data types supported by next version CF (1.8) http://cfconventions.org/cf-conventions/cf-conventions.html#_data_types and the guidance here https://github.com/cf-convention/cf-conventions/issues/191#issuecomment-514688194, I would say that in general, no.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Unidata/netcdf-java/issues/126?email_source=notifications&email_token=AAEVZQF7AA75NPZGDP42O23Q3YX6TA5CNFSM4IZAIHRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH7BNIQ#issuecomment-570300066, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEVZQA47YV5KL5PMJVC6NTQ3YX6TANCNFSM4IZAIHRA .

lesserwhirls commented 4 years ago

Can the netcdf-c library read any hdf5 or just netcdf4 ?

Just netcdf-4 as far as I understand, although better flexibility in the C library is a desired feature.

I think the thing to do is implement a new HDF5 IOSP (directly using JNA, or indirectly by using JHI5), and at least verify that the netCDF-4 spec (perhaps better named the netCDF HDF convention), remains documented thoroughly enough.

I would be tempted to go with using direct JNA calls over JHI5. That would give us an “on par” implementation in terms of netcdf-4 reading in addition to the current completely independent inplimentation.

JohnLCaron commented 4 years ago

I need to think about all that more. But just to clarify, Im just volunteering to update the pure Java code.

On Tue, Jan 7, 2020 at 7:12 PM Sean Arms notifications@github.com wrote:

Can the netcdf-c library read any hdf5 or just netcdf4 ?

Just netcdf-4 as far as I understand, although better flexibility in the C library is a desired feature.

I think the thing to do is implement a new HDF5 IOSP (directly using JNA, or indirectly by using JHI5), and at least verify that the netCDF-4 spec (perhaps better named the netCDF HDF convention), remains documented thoroughly enough.

I would be tempted to go with using direct JNA calls over JHI5. That would give us an “on par” implementation in terms of netcdf-4 reading in addition to the current completely independent inplimentation.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Unidata/netcdf-java/issues/126?email_source=notifications&email_token=AAEVZQHK4DMOAF4CF4TGBDTQ4UZB7A5CNFSM4IZAIHRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIK6IFY#issuecomment-571859991, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEVZQFK24J4WDAKEPB3XATQ4UZB7ANCNFSM4IZAIHRA .

DennisHeimbigner commented 4 years ago

FYI The netcdf-c library can read some (but not all) HDF5 files created outside of netcdf-4.

lesserwhirls commented 4 years ago

I need to think about all that more. But just to clarify, Im just volunteering to update the pure Java code.

Totally. Any JNA based HDF5 IOSP will be my special level of...well...there's a south park joke in here somewhere...ah, yes, Detroit.

FYI The netcdf-c library can read some (but not all) HDF5 files created outside of netcdf-4.

Does it expose enough of HDF5 to build a full HDF5 IOSP? If not, then calls to the HDF5 library are probably the right level for the IOSP.

DennisHeimbigner commented 4 years ago

Does it expose enough of HDF5 to build a full HDF5 IOSP?

Not even close.