aodn / imos-toolbox

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

DepthPP dialog loses 'from PRES_REL measurements' option #655

Open BecCowley opened 4 years ago

BecCowley commented 4 years ago

@ocehugo, I think this is a bug. When the depthpp dialog window comes up, it allows you to choose between 'from nearest pressure sensors' and 'from PRES_REL measurements'.

If the dialog is populated from the previous run of the toolbox, for the sensors where I have selected 'from nearest pressure sensors', I am unable to switch back to 'from PRES_REL measurements'. I have to perform a complete reset to default mapping and re-select the options for every instrument.

For instruments that don't have a pressure sensor, the behaviour makes sense. But for instruments that do have a pressure sensor (eg Aquadopps), where I decide to use or not use the pressure measurements, the behaviour doesn't make sense.

ocehugo commented 4 years ago

@BecCowley - yes - appears a simplification bug. Quick question, this is a second run on a previously processed dataset right? Kind of, I imported the data, selected some options, but I need to redo some stuff (like pressures at some instruments).

The problem here seems related to the fact that we store these DepthPP gui parameters as a single entity, that is used as "default" for all the chained datasets (i.e. parameters are equal for all datasets after the first).

When you reload the data (and the respective ppp file), only the last one is remembered and you have to redo all options again at the DepthPP dialogues. am I correct?

If the dialog is populated from the previous run of the toolbox, for the sensors where I have selected 'from nearest pressure sensors', I am unable to switch back to 'from PRES_REL measurements'.

I assumed you meant that you need to select them again and again, not that the option is not available/shown anymore. correct?

If it's what I suspect, this appears as simple as storing the state of the user-selected options better, maybe with some kind of identification number/instrument name/ so we can reload them again correctly.

BecCowley commented 4 years ago

@BecCowley - yes - appears a simplification bug. Quick question, this is a second run on a previously processed dataset right? Kind of, I imported the data, selected some options, but I need to redo some stuff (like pressures at some instruments). @ocehugo, yes that is correct. I have to re-start the toolbox quite a few times to get the timing offsets correct, the planned depths checked etc. And decide if I need to calculate depth from adjacent instruments or use the on-board sensor. You don't know the quality of the pressure data until you've checked it in the toolbox....

The problem here seems related to the fact that we store these DepthPP gui parameters as a single entity, that is used as "default" for all the chained datasets (i.e. parameters are equal for all datasets after the first).

When you reload the data (and the respective ppp file), only the last one is remembered and you have to redo all options again at the DepthPP dialogues. am I correct?

The last setting for all the instruments is remembered. But to change one back to 'from PRES_REL', I have to reset all the instruments and re-select what I wanted for each.

If the dialog is populated from the previous run of the toolbox, for the sensors where I have selected 'from nearest pressure sensors', I am unable to switch back to 'from PRES_REL measurements'.

I assumed you meant that you need to select them again and again, not that the option is not available/shown anymore. correct?

The option 'from PRES_REL' is not available anymore. I cannot select it at all for those instruments where I have selected the other option. I have attached some screen shots.

The full screen with my selections for each instrument: Screen Shot 2020-04-07 at 9 10 04 am When I try to change an instrument's depth pp from 'from nearest pressure sensors' to 'from PRES_REL' (the option is not available) Screen Shot 2020-04-07 at 9 10 27 am When I try to change an instrument's depth pp from 'from PRES_REL' to 'from nearest pressure sensors' Screen Shot 2020-04-07 at 9 10 43 am

ocehugo commented 4 years ago

You don't know the quality of the pressure data until you've checked it in the toolbox....

Yeap, I asked because I believe your case is overwriting the GUI options based on the ppp file content.

The last setting for all the instruments is remembered. But to change one back to 'from PRES_REL', I have to reset all the instruments and re-select what I wanted for each. The option 'from PRES_REL' is not available anymore. I cannot select it at all for those instruments where I have selected the other option. I have attached some screen shots.

Got it - thanks for the clarification. Your last screenshots clear up my doubts.

quick test: If you move lines 160-162 in DepthPP to line 142, the problem is solved?

ocehugo commented 4 years ago

Also, if the above solves the problem, did you notice any option previously defined changed to a default one (say PRES_REL in another instrument was moved to DEPTH or any other option?

BecCowley commented 4 years ago

I moved the lines as you said. When I first re-started, all my previously defined options were gone (except for the instruments that have no pressure, they show what I had selected). Same again when I restarted the second time. I have lost all the selections I made previously.

Unable therefore to test if this solved the problem!

BecCowley commented 4 years ago

@ocehugo, I just found another bug in this routine. If I add an instrument to the import (eg, an RDI that I haven't QC'd yet), all the instruments that have 'from nearest pressure sensors' selected have their selected instruments shifted.

Eg, I added one extra instrument to my mooring on import and the instruments to infer depth from have moved up one place.

The index of the instruments to infer depth from must be recorded instead of the instrument name, maybe?

ocehugo commented 4 years ago

When I first re-started, all my previously defined options were gone (except for the instruments that have no pressure, they show what I had selected). Same again when I restarted the second time. I have lost all the selections I made previously.

OK - Thanks. The problem here indeed is state transition/loading ppp configurations. I just don't know how a UseItsOwnPresRel=false - which also defines if the option will be displayed - is written ( reloaded) to (from) the ppp file. This is supposedly never False if the instrument got the PRES_REL variable, unless the ppp file says so. Hence, I would blame the write/load ppp file logic.

@ocehugo, I just found another bug in this routine. If I add an instrument to the import (eg, an RDI that I haven't QC'd yet), all the instruments that have >'from nearest pressure sensors' selected have their selected instruments shifted. Eg, I added one extra instrument to my mooring on import and the instruments to infer depth from have moved up one place.

Yeap - it's related to this issue of loading ppp config. We store the index instead of the instrument input file. The code is clearly not ready to deal with modifications after loading the previous options - this option is working only as a "fallback" in the sense of a crash, easy reload,etc. Apparently, the logic to blame is the +1/-1 shifts in the callback functions.

PS: We can ping-pong here but it would be quicker if I'm at the same level - I can't easily reproduce atm - I'll wait for your files/csvs from the other issues.

BecCowley commented 4 years ago

PS: We can ping-pong here but it would be quicker if I'm at the same level - I can't easily reproduce atm - I'll wait for your files/csvs from the other issues.

I have sent the files via email - last week. I have resent to the correct address this time!