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

Allow for replacing data in a Fit Page #1537

Closed
rozyczko opened this issue May 12, 2020 · 27 comments
Closed

Allow for replacing data in a Fit Page #1537

rozyczko opened this issue May 12, 2020 · 27 comments
Labels
Contraints Issues pertaining to constraints Enhancement Feature requests and/or general improvements Major Big change in the code or important change in behaviour
Milestone

Comments

@rozyczko
Copy link
Member

Follow up to the discussion in #1490 and our biweekly call.

The first step to copy/paste constraints project is to allow for replacing existing data in a Fit Page.
This could be done similarly to Append to new plot, with the "Send To" button allowing for sending data to an existing page (but by default creating a new one).

@rozyczko rozyczko added Contraints Issues pertaining to constraints Enhancement Feature requests and/or general improvements Major Big change in the code or important change in behaviour labels May 12, 2020
@rozyczko rozyczko added this to the SasView 5.0.3 milestone May 12, 2020
@m2cci-NMZ
Copy link
Contributor

Would adding an action to the data context menu in the DataExplorer window do the trick?

test

@rozyczko
Copy link
Member Author

rozyczko commented May 14, 2020

Since this functionality is (probably?) not very common, hiding it in the right click context menu is the right thing. I would not like to make the main GUI elements more complex without real need.

@smk78
Copy link
Contributor

smk78 commented May 14, 2020

Hmm. My personal feeling is that the data context menu is too obscure for a user. Might be worth putting this out on Slack for some other opinions?

Could there be a checkbox next to the Batch Mode one that says 'use current FitPage', instead?

@RichardHeenan
Copy link
Contributor

(Steve just reply whilst I was typing this) The ability to swap data attached to a fitpage will be generally very useful, even for a normal single page fit without constraints, so I would rather the feature was not hidden in a right click (though it could be there as well) but shows somewhere obvious for the novice user.

So in the "send data to" drop down rename to "Fitting (new page)" and add a new line for "Fitting (current page"), or perhaps the checkbox as Steve suggested.

@m2cci-NMZ
Copy link
Contributor

My original idea wasn't to hide the functionality 😃
It's just that the workflow when using "send data to" usually involves a list of one or several datasets, which creates one or several FittingWidget
Here it's slightly different as one would like to send just one specific dataset to one specific existing FittingWidget.
The checkbox or drop down menu would work as well, I'll just have to add a mechanism checking that the user has selected just one dataset in the DataExplorer

@RichardHeenan
Copy link
Contributor

Good point! Yes we will have to check that only one data set is involved, or just move the first one ?

@m2cci-NMZ
Copy link
Contributor

A warning interrupting flow and prompting the user to select only one dataset should do it. I'm afraid that sending the first one could be a little confusing for a novice user 😄

@butlerpd
Copy link
Member

Sorry for jumping in late but just would like to third the sentiment that this is likely to be a very popular feature rather than rarely used one for the reasons mentioned.

Regarding implementation - sent do fitting as I recall creates a new fit page for each data set being sent and associates one data set with one fit page correct? at the top of the fit page in 5.x I note there is a text label that reads Data loaded from: *dataset_name -- Would it be too confusing/difficult to make that a drop down box instead of a label when the page is created which has all data sets currently loaded as options? Then there could only ever be one data set?

Just a thought based on a concept that was discussed for 4.x many years ago (but never implemented ... or even completely thought through as to whether it was or not a good idea 😃

@butlerpd
Copy link
Member

Further thoughts:

The above suggestion would create an new concept I guess (is it good or bad? not sure.. thinking at the moment it could be good?). We now would be differentiating between starting a new Fitpage by sending data to .. fitting, PR, etc from working within one Fitpage where we may change models, fitting algorithms and even data sets.

The more I think about it, the more I'm liking that workflow concept .. but fair warning I had not so much sleep last night 😄

@smk78
Copy link
Contributor

smk78 commented May 14, 2020

Version 3.1.2 actually had the dropdown data selection box on the FitPage, you just couldn't use it, it was greyed out!

@smk78
Copy link
Contributor

smk78 commented May 14, 2020

I admit, the possibility of having different perspectives (fitting, inversion, etc) open at the same time would be attractive...

@butlerpd
Copy link
Member

3.1.2? Interesting .. so I remembered correctly that it was a thought at one point anyway :-)

@RichardHeenan
Copy link
Contributor

A drop-down on the FitPage would be good, but should we only populate it with the "top level" imported data, or the whole plethora of locally computed workspaces ? Perhaps the latter could all be towards the end of the list ? (So we can fit a different model to calculated data, or to a difference?)

@m2cci-NMZ
Copy link
Contributor

Actually the drop down table is there, it's only active when you are in batch mode.
I'll activate it for non batch fits and populate it with the imported data :-)

@rozyczko
Copy link
Member Author

I have a feeling this is going to be a nightmare. Replacing existing data without changes to underlying model is one thing, but allowing the fit tab to know about all existing datasets is very tricky in terms of keeping proper updates between DE (Data Explorer) and FT (FitTab).
Also, I thought the idea was to dissociate data manipulation (DE) from acting on data (perspectives), which leads to a well defined working pipeline (load -> send -> operate) which was the original design.
Doesn't adding the dropdown control of data choice in the FitTab negate the need for the data explorer Send To button then? The followup to the above decision is that each perspective will know of every dataset and will be able to modify it, rather than just act on data sent by the "general manager". This is a pretty radical change of the way SasView works, I feel.
If we are going to redesign the whole workflow, then we probably need to make some fundamental decisions on the structure of the code before hastily modifying core functionality...
(my $.02)

@rozyczko
Copy link
Member Author

Right. I might have overreacted a bit - the change can be done without sacrificing the core data flow but still the question of logic of how to deal with multiply selected datasets being sent to fitting will work. We might start with allowing the overwrite of the existing data in the Fit Panel - the original request. This should be straightforward. The rest (multiple datasets listed in one Fit tab) can be made a separate discussion issue.

@m2cci-NMZ
Copy link
Contributor

Ok, for now I'll just add an mechanism allowing the replacement of data attached to the fit page :-)
It is indeed a nightmare to get the fitting perspective know about all the datasets that are loaded and get list updated properly.

@butlerpd
Copy link
Member

A drop-down on the FitPage would be good, but should we only populate it with the "top level" imported data, or the whole plethora of locally computed workspaces ? Perhaps the latter could all be towards the end of the list ? (So we can fit a different model to calculated data, or to a difference?)

Ah yes. I forgot about that related issue. Can we have a design discussion about the infinite number of sub-data sets that appear in the data manager? Originally we only had some information there. Then we added the PQ, SQ separation of the I(Q) fit, then the desmeared and smeared versions and onwards. It seems at this point that for every iteration of parameters now gets saved there. So if I play around manually with parameter, even before my first fit I can have a dozen or more subsets. I understand that for being able to implement a history/go back feature this is important but most of these are useless to the user and just make it impossible to quickly find what one is looking for. I don't have a clear answer hence not putting in ticket at this point. Maybe I should create a separate ticket anyway to better enable discussion?

@butlerpd
Copy link
Member

I have a feeling this is going to be a nightmare. Replacing existing data without changes to underlying model is one thing, but allowing the fit tab to know about all existing datasets is very tricky in terms of keeping proper updates between DE (Data Explorer) and FT (FitTab)

Ah .. good point. I agree.... I did suggest that I did not know if it was a good or bad idea - apparently it was a bad one :-) On the other hand I don't think it breaks the original workflow concept (depending on how it was implemented) just enhances it... but perhaps a moot point now :-)

@RichardHeenan
Copy link
Contributor

Though I am of course very much in favour of being able to replace the data attached to a FitPage, there is also the tricky issue of the plots related to that Fitpage. I am presuming that all of them would immediately be updated to show new data (which might have a different Q range or vertical scale) but with the previous fit - which would be fine by me, and shows the user where they need to go next as it were.
The alternative would be to "freeze" (or even delete) the original plot(s) and force an entirely new plot from scratch, which again would be OK by me, but could get the "theories" data sets and labelling into a mess.

@smk78
Copy link
Contributor

smk78 commented May 19, 2020

I would like to vote for the first of @RichardHeenan 's suggestions, on the grounds it is the logical one; the data have changed by not the theory/model.

@rozyczko
Copy link
Member Author

rozyczko commented May 19, 2020

So,

  1. load dataset d1 and send it to FitPage1
  2. assign a model
  3. d1 gets two new children in the Data Explorer: M1 [d1] and Residuals for M1 [d1]
  4. Show Plot shows the two newly created "children" (results) datas
    This is how it works until now.
    What we want is:
  5. Load d2 and send it to the same FitPage1
  6. This unloads d1 from FitPage1. Nothing gets deleted from under d1 in Data Explorer
  7. d2 entry in the Data Explorer has now two new children (or results) for M1 [d2] and Residuals for M1 [d2]
  8. Show Plot shows plots of the above [d2] children
  9. Plot of M1 [d1] can be still accessed by selecting them in Data Explorer and using "Create New" plot.

I think the above is intuitive and fulfills Richard's requirement.

@RichardHeenan
Copy link
Contributor

Sounds good !

Presume any existing plots of M1 [d1] stay as seen prior to d2 arriving?

Would save project have any issues ? suspect not, as it knows that d2 is attached to FitPage1 ?

@rozyczko
Copy link
Member Author

Presume any existing plots of M1 [d1] stay as seen prior to d2 arriving?

They will just stay on the screen, being associated with the Data Explorer entry, not the Fit Page!

Would save project have any issues ? suspect not, as it knows that d2 is attached to FitPage1 ?

Save Project saves the current state, which at the save time has d2 in FitPage1, so no problems with saving at all.

@butlerpd
Copy link
Member

butlerpd commented May 28, 2020

@RichardHeenan I accidentally came across an old ticket of yours #1221. Is it safe to say this is the same issue and can be closed now as obsolete? Done - old issue closed by Richard

@butlerpd
Copy link
Member

@RichardHeenan or @m2cci-NMZ this seems to have been fixed in 5.0.3? but this issue remained open. Can you comment/verify that I'm not dreaming and that it is in fact OK to close this issue?

@m2cci-NMZ
Copy link
Contributor

Yes, this was indeed fixed in 5.0.3, closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contraints Issues pertaining to constraints Enhancement Feature requests and/or general improvements Major Big change in the code or important change in behaviour
Projects
None yet
Development

No branches or pull requests

5 participants