jllodra / ncdump-json

Modified ncdump to output data in json format
85 stars 28 forks source link

Cast all attribute numbers to string #6

Closed ocehugo closed 2 years ago

ocehugo commented 8 years ago

Hi,

thanks for the software. I was having some problems with Attributes with numbers on:

netcdf test { dimensions: time = 1; variables: float temperature(time); temperature:valid_min = 0s ; temperature.valid_max = 0.01f;

:title = ""; }

So I hacked the ncdump source code...Don't know if it will break anything since I can't run the tests scripts (osx here and no json_verify). Just a quick and dirty fix, but works for me using python json.tool.

Cheers

jllodra commented 8 years ago

Hi @ocehugo, thanks for your PR,

could you please detail a bit more your problem? and, if the problem appeared for a specific netcdf file, could you point us to it?

This will help reviewing the PR, and we could add it as a test case as well.

Best

ocehugo commented 8 years ago

Hi @jlodra,

My example was wrong. I corrected the snippet above to make it consistent with the problem of this PR.

1.The problem can be reproduced in the raw example that I rewrote above. if the attributes of the variables have the following 1000f or 0d (the cdl way to provide the type (float,int,etc)) the result of the json dump is invalid since the value of the attribute is set to a number with a letter at the end.

Howto reproduce: Generate the netcdf of the snipet above and try to import the jsondump. It will fail because the attributes are set as numbers but they arent number for json readers (the "f" or "d" at the end). So to be readable it should be casted to string (double quotes). To test the json output im using ncdump-json -h -j file.nc | python -m json.tool to see if its valid or not.

"The supposed way" that i see for of ncdump-json would be to break out all attributes in new keys:value pairs for the type and value of the attribute. so if the attribute of a variable have a number followed by a letter "f","s",etc is should generate 2 new items, type (of att) and value (of att).

In the process i discovery other flaws (see new issues):

  1. ncdump-json doesn't support netcdf4 group cdl (It's include a empty {} on the top of the json file) - My older snippet with groups was not generating valid json dumps
  2. ncdump-json doesn't not generate a valid json dump if there is no global attributes (the program include an extra } at the end of the file)
jllodra commented 8 years ago

Hi @ocehugo,

there are a few things that confuse me about your pull request, looking at: https://github.com/jllodra/ncdump-json/pull/6/files, double quotes were being correctly added (lines 457 and 462, so why adding another extra pair of double quotes in 461?). You might be correct for other cases like 408 and 412 (it is possible I never tested NC_SHORT or NC_INT types...), but the if(is_json) case should be considered as well.

The idea of breaking up in an object with type/value keys sounds OK to me.

Regarding the issues #7 and #8 you mention, let me answer in the appropriate threads.

ocehugo commented 8 years ago

You may be right, maybe I add extra quotes where it was not needed.

I did the changes on demand. It works for my netcdf ( mostly using -h , since i only need the headers). I also don't have a lot of time since my modifications works for me (~15 header files worked and imported by python!), so i'm happy with it by now.

Looking at your testing suite I believe that the run_tests.sh would benefit from a json reader/tester, since you are not testing at all the json output (only if the ncdump-json program runs ok) (another issue!?). You could use python json as I do, but this would add a dependency that maybe you are not happy with it.

Thanks again

jllodra commented 8 years ago

Hi @ocehugo,

I'll take a look at the code and try to fix that, and also include some more tests.

The run_tests.sh script uses a program called json_verify (https://github.com/jllodra/ncdump-json/blob/master/tests/run_tests.sh#L26), but I'll try to remove that dependency and use something that is bundled with the OS, and add more tests as well.

Thanks for all your research on these issues.

jllodra commented 8 years ago

Hi @ocehugo, just commited some changes 9776e37c006816526b215257bdfcccbbf79a7ad2.

I think this fixes the attribute issues (without having to convert numbers to strings). I also made some improvements to tests and python's json.tool is used now.