Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESS_GUI: Save/Load Corfunc and Invariant Perspectives #1675

Merged
merged 20 commits into from
Nov 24, 2020

Conversation

krzywon
Copy link
Contributor

@krzywon krzywon commented Sep 22, 2020

This allows data from the Corfunc and Invariant perspectives to be saved and loaded in project and analysis files. This also fixes a few other issues associated with loading projects. Unit tests added for all serialization methods.

Closes #1526 : All perspectives can now be saved and loaded
Fixes #1657 : Loading project files without fitting perspective data now load properly
Fixes #1655 : Corfunc and Invariant perspectives are now able to delete and swap data as needed
Fixes #1674 : The data names are now reset when loading a new project
Fixes #1539 (Unintended fix) : The name now appears the first time you send data to the Corfunc perspective

krzywon added 17 commits August 6, 2020 09:36
…d use Fitpage swap mechanism when loading projects. refs #1657
…o Invariant and Corfunc perspectives. Fix bugs found from those tests.
…to be sure the 'isSerializable' value is set properly.
# Conflicts:
#	src/sas/qtgui/MainWindow/DataExplorer.py
#	src/sas/qtgui/Perspectives/Corfunc/CorfuncPerspective.py
…om Corfunc and add verification when swapping data in Corfunc.
@krzywon krzywon added this to the SasView 5.0.4 milestone Sep 22, 2020
@krzywon krzywon requested review from rozyczko and smk78 September 22, 2020 19:52
Copy link
Contributor

@pkienzle pkienzle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks okay but I didn't test.

# Tell the MdiArea to close the container
self.parentWidget().close()
# Tell the MdiArea to close the container if it is visible
if self.parentWidget():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat suspicious: the code requesting close gets here so the widget exists, but it is not bound to a parent so it doesn't get removed.

Is this a dangling pointer, where the widget was already removed, but some other function is saying to remove it again? Or is this a zombie window which might reappear later even though you told all windows to close?

The same code is in invariant and inversion, so update/ignore in all places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parentWidget for a perspective will be a widget in the main window, but only if that perspective is active. All non-active perspectives are removed from the main window and held in the self.loadedPerspectives variable of GuiManager.

Copy link
Member

@rozyczko rozyczko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected.
Code is ready, too.

Collects all active params into a dictionary of {name: value}
:return: {name: value}
"""
return {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably query the model (self.model) rather than UI elements.

@smk78
Copy link
Contributor

smk78 commented Oct 30, 2020

Built and tested this branch on W10/x64.

Performed correlation function analysis on \test\1d data\ISIS_98929.TXT (load data, send to Corfunc, Calculate background, Extrapolate, Transform & Extract Parameters). Then did Save Project. Restarted SasView and did Load Project.

Everything comes back except the Real Space Correlations plot. See below.
image
Which is a bit odd because it has re-drawn the Scattering Data plot.

However, it does know about the transformed data, because if you do Create New it plots it:
image

@smk78
Copy link
Contributor

smk78 commented Oct 30, 2020

Performed Invariant analysis on \test\1d data\Ludox_silica.xml (load data, send to Invariant, changed background, Calculate). Then did Save Project. Restarted SasView and did Load Project.

Seemed to work as expected.

@smk78
Copy link
Contributor

smk78 commented Oct 30, 2020

After performing the test of Invariant project saving/loading above, I then, without closing the session (ie, SasView contained the reloaded Invariant project), loaded ISIS98929.TXT and performed Correlation Function analysis. I then saved the session to a new project. This took a while but no cause was immediately obvious. But when I closed SasView I found my Command Prompt window filled with the following messages (so many they filled the window buffer):

INFO: data cannot be serialized to json: <class 'numpy.float32'>

Closing SasView, restarting it, then re-loading the saved invariant-then-corfunc project file did work, and I could toggle between the two analyses using the Analysis menu option.

I then went 'the whole 9 yards', loading AOT_Microemulsion-Drop_Contrast.xml and fitting it with 10% Lognormal PD aswell. I then saved the session to a new project, closed SasView, restarted it, and re-loaded the invariant-then-corfunc-then-fitting project file. This also worked, though none of the fitting plots (data, residuals or size distribution) appeared until I did Show Plot in the FitPage. Interestingly the 'Create New' plot I did after doing invariant-then-corfunc was displayed.

project_files.zip

Copy link
Contributor

@smk78 smk78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in summary: this PR does reinstate the ability to save correlation function or invariant analyses to projects, either singly, or in combination with one another, or in combination with fitting analyses without any apparent ill effects. For this reason I will approve the PR.

However, as my comments above demonstrate, there is some weirdness regarding what plots are redrawn and which are not.

@butlerpd
Copy link
Member

butlerpd commented Nov 9, 2020

So the question now seems to be whether @krzywon thinks we should merge and create new tickets based on the testing or whether some or all of these issues are "obvious" and should be fixed as part of the PR. To summarize my understanding of the issues (@smk78 can correct)

  1. Real Space Correlation plot does not populate properly on loading project (but can be manually forced to plot)
  2. saving PR and Invariant (at least in the sequence described) is a bit slow to save but otherwise works perfectly fine "functionally." However there are a ton of "data cannot be serialized" info messages (probably from a loop?). As info they are not errors I guess but perhaps this at least can be fixed by moving the info statement outside the loop?
  3. Fitting plots do not show on loading fitting project at least when part of a Invariant+corfunc+fitting saved project. Question:What is the current behavior of fitting save? is this part of the problem in not seeing plots in fitting till doing a fit or asking to show plot?

@krzywon
Copy link
Contributor Author

krzywon commented Nov 10, 2020

  1. The Corfunc correlation plot is now updated and the buttons are enabled properly with my latest push.
  2. The 'data cannot be serialized' only happens when data loaded from a project is saved again and only effects the data in plots. I wasn't able to solve it here and opened a new issue Resaving Loaded Projects - 'data cannot be serialized' #1706.
  3. Invariant plots are now plotted, but the plots are still effected by issues Invariant and the infinite multiplication of plots #1541 and Crosstalk between Corfunc and Invariant perspectives #1542.

@butlerpd butlerpd merged commit 6f32287 into main Nov 24, 2020
@krzywon krzywon deleted the ESS_GUI_serialize_corfunc_invariant branch November 24, 2020 14:27
@smk78 smk78 mentioned this pull request Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment