geomagpy / magpy

MagPy (or GeomagPy) is a Python package for analysing and displaying geomagnetic data.
BSD 3-Clause "New" or "Revised" License
46 stars 27 forks source link

plotstream and streamlist ( with current plotstream in it ) can be out of sync #118

Open stephanbracke opened 2 years ago

stephanbracke commented 2 years ago

While cleaning a baseline plot by removing the flagged points, it will update the current plotstream with nan on the place of the flags. https://github.com/geomagpy/magpy/blob/ae91462f1644ebf89fbe570ce1cf67d1d89eb926/magpy/gui/magpy_gui.py#L5962 However on that moment there is also self.streamlist and on index 0 (if it is the only loaded data) you find the same stream but without the flag removal. When we now click on fit we see a fit happening by using the plotstream (with flag removal ) https://github.com/geomagpy/magpy/blob/ae91462f1644ebf89fbe570ce1cf67d1d89eb926/magpy/gui/magpy_gui.py#L4515-L4519 This one is ok. If we now load a data file, the baseline button becomes available. By clicking on this button we will arrive in following code https://github.com/geomagpy/magpy/blob/ae91462f1644ebf89fbe570ce1cf67d1d89eb926/magpy/gui/magpy_gui.py#L4913

With abstream being

https://github.com/geomagpy/magpy/blob/ae91462f1644ebf89fbe570ce1cf67d1d89eb926/magpy/gui/magpy_gui.py#L4893

and later on this abstream = bas is used to fit https://github.com/geomagpy/magpy/blob/ae91462f1644ebf89fbe570ce1cf67d1d89eb926/magpy/stream.py#L2451

This abstream is however the stream out of the self.streamlist that hasn't taken into account the dropped flags and will result into a faulty baseline fit. As a work around we saved the cleaned plotstream into a magpy cdf and after closing magpy and reloading this cdf file the self.streamlist will be correct and we can use a correct fit on the data. As another test to be sure it was caused by these streams not being insync I quick/dirty inserted the line just after line 5962 plotstream.remove_flagged()

        self.streamlist[self.currentstreamindex] = self.plotstream

If we are sure that the currentstreamindex always correspond with the current plotstream we could accept to solve it this way but for this I need to investigate further. However we need to make sure when plotstream is changed and it has a possible use later on afterwards the stream that correponds in the streamlist should also be synced with this plotstream

leonro commented 2 years ago

Have you tried to add the current working state to the memory. i.e. add a new instance to self.streamlist? This should be done directly after the flags have been removed.

Just checked that: at the moment the baseline dictionary is only updated when initially loading the file, not when updating baseline data using "add to working state". I will work on a solution...

leonro commented 2 years ago

Each instance of basevalue data (as soon as added to the memory) will now get an index for selection. Solved in 3fa747645cae3dec0cb7170fccb1d7d59060ee3d

stephanbracke commented 2 years ago

This is still a problem just tested it on branch gui-fixes/linux

stephanbracke commented 2 years ago

Have you tried to add the current working state to the memory. i.e. add a new instance to self.streamlist? This should be done directly after the flags have been removed.

Just checked that: at the moment the baseline dictionary is only updated when initially loading the file, not when updating baseline data using "add to working state". I will work on a solution...

I just tried when you push on the add current working state to memory it will work but what is confusing to the user is that you can push on the baseline button while you forgot to push to the add to current ... then you get a baseline fit without taken into account the dropped flags

stephanbracke commented 2 years ago

I think I have a simple solution to solve this issue For the moment you have in the magpy_gui there is an InitialRead Method Here we have https://github.com/geomagpy/magpy/blob/ae91462f1644ebf89fbe570ce1cf67d1d89eb926/magpy/gui/magpy_gui.py#L1875

If you remove the copy the plotstream and streamlist will be aligned so everything you do on the plotstream will be reflected in the streams as well

leonro commented 2 years ago

If I remove the copy() method here then I will loose the functionality of the "restore" button on the Data panel. This "restore" possibility however is something which at least I use quite frequently. I personally would prefer to keep it as suggested above which however requires the " add ... to memory". Eventually one could add some "don't forget to save" notification box, if a data set has been flagged etc and the user wants to open a new data set.

stephanbracke commented 2 years ago

Ok, I didn't know it was linked with the restore. I investigated the code further and their are other ways of resolving it

Second one is the most easy for the user new dataset will be loaded but before that we update the streams with the cleaned plotstream To do that we go to the method InitialRead

we check if streamlist is not empty and if so we update the streamlist with the latest plotstream here is it situated

https://github.com/geomagpy/magpy/blob/6a560b1cf7b9a0a76604250cbdb3ccf3aa2595a9/magpy/gui/magpy_gui.py#L1868-L1877

you can insert just as first lines

       if len(self.streamlist) > 0:
            self.streamlist[self.currentstreamindex] = self.plotstream

in method InitialRead

If you however don't want this but you prefer that users always push on the add to memory stream button you could eliminated following lines https://github.com/geomagpy/magpy/blob/6a560b1cf7b9a0a76604250cbdb3ccf3aa2595a9/magpy/gui/magpy_gui.py#L1661-L1671

Just comment them and the baseline with the current plot will not be added, so button on baseline will not be available when loading a new plot stream