aodn / imos-toolbox

Graphical tool for QC'ing and NetCDF'ing oceanographic datasets
GNU General Public License v3.0
45 stars 30 forks source link

Contour fixes #774

Closed hugo-sardi closed 1 year ago

hugo-sardi commented 2 years ago

This extends the oceanContour parser to be able to read BinMapped processed files.

It may be a bit rough still, so please check it out.

An example file is available here (please remove the .zip extension since I had to fake so github would accept the upload): Avg_001.VTC.DSEL.nc.zip

hugo-sardi commented 2 years ago

@sspagnol

sspagnol commented 2 years ago

Works for me. Note you also need to do the same "[prefix ]" for the bin mapped mat import case in get_varmap()

hugo-sardi commented 2 years ago

Works for me. Note you also need to do the same "[prefix ]" for the bin mapped mat import case in get_varmap()

yeap - that was why I didn't push the code before - I think I added a TODO comment somewhere about this. Maybe just a copypasta from the above code lines will do!?

sspagnol commented 2 years ago

Works for me. Note you also need to do the same "[prefix ]" for the bin mapped mat import case in get_varmap()

yeap - that was why I didn't push the code before - I think I added a TODO comment somewhere about this. Maybe just a copypasta from the above code lines will do!?

Yes, that is what I did when I just tested it :)

And this will also cover https://github.com/aodn/imos-toolbox/issues/773

sspagnol commented 2 years ago

And just to check that HEIGHT_ABOVE_SENSOR isn't handling downward facing instrument? So would need something before assigning to dimensions{2}.data, kind of like

adcpOrientation = mode(bitand(bitshift(uint32(get_var('status')), -25),7));
if adcpOrientation == 5
   % case of a downward looking ADCP -> negative values
   z= -z;
end

and add 'status' to var_mapping.

hugo-sardi commented 2 years ago

And just to check that HEIGHT_ABOVE_SENSOR isn't handling downward facing instrument? So would need something before assigning to dimensions{2}.data, kind of like

AFAIK, if the instrument is downward facing, the parser will consider it upward-facing simply because there is no orientation check - I didn't have any downward-facing instruments at the time.

I explicitly check that all z values read from the file are positive in the code, but I can't remember why. Maybe for tests or the expectation that I would handle the orientation before reading the vertical variable.

Also, as far as my memory goes, the user can select autodetection of orientation too, so probably we ought to be over-restrictive in handling this kind of stuff (since we lack file diversity).

sspagnol commented 2 years ago

custom_magnetic_declination is set via custom_magnetic_declination = logical(meta.magDec); And I would have thought that the logic in build_magnetic_variables(custom_magnetic_declination) would mean ucur_name, vcur_name would be returned as 'UCUR_MAG', 'VCUR_MAG' bit it seems to reversed ('UCUR', 'VCUR' returned). Am I interpreting this wrong?

hugo-sardi commented 2 years ago

custom_magnetic_declination is set via custom_magnetic_declination = logical(meta.magDec);

Yes, but this is now a bit outdated. The problem here is that meta.magDec is NOT coming from the magnetic declination attribute. See line 194.

And I would have thought that the logic in build_magnetic_variables(custom_magnetic_declination) would mean ucur_name, vcur_name would be returned as 'UCUR_MAG', 'VCUR_MAG' bit it seems to reversed ('UCUR', 'VCUR' returned). Am I interpreting this wrong?

Yes, it is reversed. the reason is convoluted:

The problem here is that, by using a parameter that was always off, the logic is reversed. I honestly can't quite remember why I used "Intrument_user_decl". Maybe because of the decl moniker, but the description of this is not related to magnetic declination but to something not used in signatures.

The fix is relatively simple today since I now know what OceanContour do: It only outputs processing attributes if you use them.

Hence this is just a matter of changing line 194 to read the "DataInfo_transformsAndCorrections_magneticDeclination" global attribute instead and shift the logic in the magnetic declination function you mentioned. However, we probably need some extra logic around the actual value, adding attributes similar to the PP, carrying the value forward, etc.

lbesnard commented 1 year ago

closing as https://github.com/aodn/imos-toolbox/pull/793 got merged