New NavigationTool button for 1D plot #3763
Conversation
ChristianWoegerbauer
force-pushed
the
NavTool
branch
from
2ad0d81 to
68d6110
Compare
ChristianWoegerbauer
force-pushed
the
NavTool
branch
from
a5e1263 to
dea9d99
Compare
src/sas/qtgui/Plotting/Plotter.py
Outdated
| """ | ||
| Add a new plot of self._data to the chart. | ||
| """ | ||
| self.lable_name = data.name |
There was a problem hiding this comment.
I suppose it should be label instead of lable
rozyczko
left a comment
rozyczko
left a comment
There was a problem hiding this comment.
Minor comments to consider on code clarity and methods location.
|
|
||
|
|
| # This way all can be called with: | ||
| # self._actions['xxx'] | ||
| custom_icon = QtGui.QIcon() # You can load an icon here if you want e.g., QtGui.QIcon("path/to/icon.png") | ||
| custom_action = QtGui.QAction(custom_icon, "Fitting", self) |
There was a problem hiding this comment.
"Fitting" is not descriptive enough, even with the tootlip. Please consider e.g. "Send to fitting" or even better "Freeze" since this is what the operation does.
| current_file_name: str = self.parent.lable_name | ||
| search_name = current_file_name |
There was a problem hiding this comment.
Unnecessary extra variable - please use only one
| current_file_name: str = self.parent.lable_name | ||
| search_name = current_file_name | ||
|
|
||
| def find_name(model, target_name: str, column: int=0) -> int: |
There was a problem hiding this comment.
Please use camelCase for method names in classes derived from QObject.
| model = self.parent.parent.model | ||
| row_index_parent, row_index_child = find_name(model, search_name) | ||
| data_dir = self.parent.parent.model.item(row_index_parent) | ||
| data = data_dir.child(row_index_child) | ||
| new_item = self.parent.parent.parent.filesWidget.cloneTheory(data) | ||
| model.beginResetModel() | ||
| model.appendRow(new_item) | ||
| model.endResetModel() | ||
| self.parent.parent.parent.filesWidget.sendData(None, [new_item]) |
There was a problem hiding this comment.
Doing all these operations in PlotterBase, by referencing parent.parent.parent.... is smelly.
The best idea is to implement something like freezeDataWithName in DataExplorer, so no such object path traverse is taking place.
Here, you would just find out the name (line 43) and pass it to a signal, which DataExplorer would act on and do all these operations locally.
|
|
||
| self.contextMenu = QtWidgets.QMenu(self) | ||
| self.toolbar = NavigationToolbar(self.canvas, self) | ||
| self.toolbar = CustomToolbar(self.canvas, self)#NavigationToolbar(self.canvas, self) |
There was a problem hiding this comment.
please remove commented out code
| self.parent.manager.updateModelFromPerspective(new_item) | ||
|
|
||
| items_for_fit.append(new_item) | ||
| self.new_item = new_item |
There was a problem hiding this comment.
self.new_item isn't used - please remove
butlerpd
left a comment
butlerpd
left a comment
There was a problem hiding this comment.
Testing the installer using a windows 11 machine I found the following issues. Some are questions of easthetics, some are "not great" but don't break and one is an error that I think must be addressed.
- while loading a 2D data, creating a 1D slice and then clicking the
Send to fittingbutton on that works as advertised, if you startup sasview, load 1D data and then create a new graph with one of those then clickSend to fittingthis error appears (this is the case even if I start with a slicer.):
10:46:27 - ERROR: Traceback (most recent call last): File "sas\qtgui\MainWindow\DataExplorer.py", line 829, in freezeFromName AttributeError: 'NoneType' object has no attribute 'child'- If you click on
Send to fittingseveral times the same data is created several times. This may be the correct behavior in that the data in a given plot may change? but if it hasn't and the user forgets they have done it, the number of same data files piles up in the explorer? - I note that because this is done in the navigator and the new action is right justified, the x,y cursor update is now to the left of that action (so no longer right justified itself). However on 2D data, which does not have the new action the cursor update still shows as right justified. Can the new action not just be left justified at the end of the existing actions? so to the right of the save icon?
Otherwise, sending data directly to fitting or inversion etc works as before. Also the new action on 1D data works for whatever analysis is loaded (including inversion and invariant) and fitting works .... once we get past the error described in one above.
Finally I note that after playing around a while we can get past that error and then Send to data works properly on all 1D plots as expected
|
Playing with this, I see that if one has a figure with several curves only the last one is sent to the data explorer (if not there) and loaded into a new FitPage. I agree this is the best behaviour, but I would suggest to inform about it using the tooltip (e.g. modify to say Send the last data set to Fitting). If the functionality is described in the documentation, update it also there. |
|
Thanks @ChristianWoegerbauer. I'm flying back tomorrow so it will likely be Thursday before I can look |
butlerpd
left a comment
butlerpd
left a comment
There was a problem hiding this comment.
Thanks @ChristianWoegerbauer. This is a very nice feature addition and is working pretty smoothly IMO. I only found one minor issue besides what @gonzalezma identified.
Regarding the multiple curves in a single plot, I would think that sending all curves to the fit panel would be the expected behavior, each to its own tab. That would be consistent with how most of SasView works. However, if that is too much of an ask, I'm happy with Miguel's suggestion of adding that information to the tooltip and updating the documentation issue to indicate the need to include this.
The other issue I found is that if you send the single 1D data from the same plot several times, which could be legitimate if from a slicer for example where the slicer parameters are changed each time, 4 copies of that slicer are promoted to the DataExplorer, all having the same name with an additional _@### tacked on at the end to differentiate them. This part works.
However, the name sent to all fit tabs is exactly the same name which is the base name without the extra _@###. This makes it impossible to figure out which curve I am actually fitting. I am not sure why that is truncated from what is sent to the panel, but also I'm not sure where that _@### is coming from? is that being added in the code change? In general if a second data set is sent to the data explorer with the same name it gets given an identifier [1] for the second [2] for the third and so on. These do get copied to the title of the FitPage. Or is the file being sent to the FitPage directly and not using the DataExplorer machinery?
|
@butlerpd: The ending |
|
Thanks @ChristianWoegerbauer. Indeed, if the nomenclature problem comes from the underlying infrastructure you are using then it should definitely not be fixed here. It should be done in a separate PR. I'll try to make an issue for that. Cool that you were able to make it work for all plots. I will try to test it sooon |
butlerpd
left a comment
butlerpd
left a comment
There was a problem hiding this comment.
Functionality looks really slick now! I note that you clicked that "there is a issue to add the documentaion" I was not able to find that? can you give the issue number? This should be docummented in the Plotting Data/Models page of the documentation (under Working with SasView)
Description
When using a slicer, a 1D plot opens. An action button has been added to the NavigationToolbar, which directly sends the 1D data to the fitting perspective. Furthermore, the 1D data is added to DataExplorer as a new datafile.
How Has This Been Tested?
LoadData -> e.g. src\sas\example_data\2d_data\SILIC010.DAT -> Send to Fitting -> Category e.g. Sphere, Model name e.g. Sphere -> Compute/Plot -> right_click on 2D plot and choose slicer -> 1D plot opens -> in NavigationToolbar a new action called "fitting" appears" -> when pressing the ActionButton the data is send to the fitting persepctive in a new tab and is added to DataExplorer as a new datafile.
Review Checklist:
[if using the editor, use
[x]in place of[ ]to check a box]Documentation (check at least one)
Installers
Licensing (untick if necessary)