Plotting refactor #2940
Plotting refactor #2940
Conversation
|
These mockups are what was discussed for displaying 2d data |
lucas-wilkins
left a comment
lucas-wilkins
left a comment
There was a problem hiding this comment.
Mostly pretty good, could do with a few more docstrings and using some of the newer python capabilities (newer typing, generics), put types on parameters, don't commit generated code, look at iteration over collections using for loops and at enumerate and zip.
James is quite impressed given you're not a CS student
plotting_refactor/DataCollector.py
Outdated
| #class to keep track of all datasets of fitpages and more | ||
| import RandomDatasetCreator | ||
| from Dataset import Dataset | ||
| from typing import List |
There was a problem hiding this comment.
Don't need to import List anymore in newer pythons
plotting_refactor/DataCollector.py
Outdated
|
|
||
| class DataCollector: | ||
| def __init__(self): | ||
| self.datasets: List[Dataset] = [] |
plotting_refactor/DataCollector.py
Outdated
| self.datasets: List[Dataset] = [] | ||
| self.datasetcreator = RandomDatasetCreator.DatasetCreator() | ||
|
|
||
| def update_dataset(self, main_window, fitpage_index, create_fit, checked_2d): |
There was a problem hiding this comment.
what are the types of these?
plotting_refactor/DataCollector.py
Outdated
| def update_dataset(self, main_window, fitpage_index, create_fit, checked_2d): | ||
| # search for an existing dataset with the right fitpage_index | ||
| existing_dataset_index = -1 | ||
| for i in range(len(self.datasets)): |
There was a problem hiding this comment.
i, dataset in enumerate(self.datasets)
| for i in range(len(self.datasets)): | ||
| if self.datasets[i].get_fitpage_index() == fitpage_index: | ||
| existing_dataset_index = i | ||
| if existing_dataset_index == -1: |
There was a problem hiding this comment.
add new line between blocks, if you like
plotting_refactor/Dataset.py
Outdated
| def get_fitpage_index(self): | ||
| return self.fitpage_index | ||
|
|
||
| def get_x_data(self): |
plotting_refactor/Dataset.py
Outdated
| else: | ||
| print("no integer") | ||
|
|
||
| def set_x_data(self, x_data): |
plotting_refactor/FitPageUI.py
Outdated
| @@ -0,0 +1,67 @@ | |||
| # Form implementation generated from reading ui file 'FitPageUI.ui' | |||
| from scipy import special | ||
|
|
||
|
|
||
| class DatasetCreator: |
| print(a.shape) | ||
| print(b.shape) | ||
| print(c.shape) | ||
| ''' No newline at end of file |
There was a problem hiding this comment.
remove, unless you're still using it.
krzywon
left a comment
krzywon
left a comment
There was a problem hiding this comment.
I've looked at DataCollector.py through MainWindow.py in my first round of code review. I will take a look at the other half next week.
plotting_refactor/DataCollector.py
Outdated
| def get_datasets(self) -> List: | ||
| return self.datasets | ||
|
|
||
| def get_data_fp(self, fitpage_index) -> Dataset: |
There was a problem hiding this comment.
This and the next 6 methods are almost exactly the same and could probably be abstracted a bit. Also, the name for your first method suggests you are getting the fitpage, but you are getting the data. get_data_by_fp is more syntactically correct.
An untested catch-all that could be a function:
def get_prop_from_object(obj, prop):
for k, v for enumerate(obj):
if hasattr(v, prop) and v.prop == prop:
return v
plotting_refactor/DataCollector.py
Outdated
|
|
||
| return x_data, y_data, y_fit | ||
|
|
||
| def get_datasets(self) -> List: |
There was a problem hiding this comment.
Getters and setters are very Java-esque. Invoke self.datasets instead, but this gives full access to the outside.
_datasets could be a private class variable instead, and then, optionally, datasets could be a property of the class to allow other logic to happen when getting/setting the value.
plotting_refactor/DataCollector.py
Outdated
| for i in range(len(self.datasets)): | ||
| if self.datasets[i].get_fitpage_index() == fitpage_index: | ||
| existing_dataset_index = i |
There was a problem hiding this comment.
Another place where you are iterating through all datasets looking for a particular item. Something as simple as holding all data in a dictionary that is generated as data is added would simplify and likely speed up the processing of all of these.
plotting_refactor/DataCollector.py
Outdated
| self.datasets[existing_dataset_index].set_y_fit(y_fit) | ||
| self.datasets[existing_dataset_index].set_2d(checked_2d) | ||
|
|
||
| def simulate_data(self, main_window, fitpage_index, create_fit, checked_2d): |
There was a problem hiding this comment.
This collector looks more like a controller than a view, but you are treating it as both in this case. I would suggest you pass in the parameters (scale, radius, height, etc.) to keep a clean separation from data calculations and GUI interactions.
| def get_data_id(self): | ||
| return self.data_id | ||
|
|
||
| class DataItem(PlotPageItem): |
There was a problem hiding this comment.
These do not need to inherit from one another. The DataItem instances should be held within a PlotPageItem instead.
plotting_refactor/DataViewer.py
Outdated
| if isinstance(self.plotTreeWidget.topLevelItem(i), TabItem): | ||
| if fitpage_index == self.plotTreeWidget.topLevelItem(i).data(0, 1).get_fitpage_index(): |
There was a problem hiding this comment.
This could be all on one line. Python evaluates each part of an if statement individually and from left to right, so there will be no cost or increase in errors by combining these.
if isinstance(self.plotTreeWidget.topLevelItem(i), TabItem) and fitpage_index == self.plotTreeWidget.topLevelItem(i).data(0, 1).get_fitpage_index():
plotting_refactor/DataViewerUI.py
Outdated
There was a problem hiding this comment.
Our .gitignore usually manages the auto-generated Python files, but that is only when they are subpackages into UI directories.
Note - keep these in the repo for now, because they help people demo the package without any extra work, but the auto-gen files should not be included in the repo once this has been integrated into src/sas.
plotting_refactor/Dataset.py
Outdated
| self.x_data = x_data | ||
| self.y_data = y_data | ||
| self.y_fit = y_fit | ||
| self.plotpage_index = plotpage_index |
There was a problem hiding this comment.
As discussed at the call today and tangentially mentioned in other comments, the data should have no knowledge of the plots, and the plots should be a view of the data. plotpage_index would be better served elsewhere.
| if isinstance(plotpage_index, int): | ||
| self.plotpage_index = plotpage_index | ||
| else: | ||
| print("no integer") |
There was a problem hiding this comment.
Your check will reject floats and string representations of integers. Values that cannot be coerced to ints will throw a ValueError.
| if isinstance(plotpage_index, int): | |
| self.plotpage_index = plotpage_index | |
| else: | |
| print("no integer") | |
| try: | |
| self.plotpage_index = int(plotpage_index) | |
| except ValueError: | |
| print("no integer") |
There was a problem hiding this comment.
I don't think it should be accessible as a string. If you use a type annotation, it would be then safe to assume that the input would be an int (obviously, someone could pass it an int, but if things go wrong, that's their fault)
plotting_refactor/MainWindow.py
Outdated
| sys.excepthook = excepthook | ||
| app = QtWidgets.QApplication(sys.argv) | ||
| window = MainWindow() | ||
| window.show() | ||
|
|
||
| ret = app.exec() | ||
| sys.exit(ret) |
There was a problem hiding this comment.
Your implementation will always run if MainWindow is imported in any way, as well as when the file it is directly called from python. Wrapping this in a function will eliminate running on import but still give you the ability to access the function from outside, if desired.
| sys.excepthook = excepthook | |
| app = QtWidgets.QApplication(sys.argv) | |
| window = MainWindow() | |
| window.show() | |
| ret = app.exec() | |
| sys.exit(ret) | |
| def main(): | |
| sys.excepthook = excepthook | |
| app = QtWidgets.QApplication(sys.argv) | |
| window = MainWindow() | |
| window.show() | |
| ret = app.exec() | |
| sys.exit(ret) | |
| if __name__ == '__main__': | |
| main() |
| return self.checkBox2dData.isChecked() | ||
|
|
||
| def index_changed(self, selected_item): | ||
| if selected_item == 0: |
There was a problem hiding this comment.
My only other comment: Python uses 'truthiness' of objects. 0 is false and 1 is true, so, unless there is a chance set_item could be something other than 0 or 1, this if/else could simplify to self.doubleSpinBox_height.setDisabled(not selected_item).
|
|
||
| return None | ||
|
|
||
| def get_data_by_fp(self, fitpage_index: int) -> Dataset: |
There was a problem hiding this comment.
A better name would just be data_by_id
…t has to be modified afterwards beyond the if statements, because 2d plotting is different from 1d plotting, as a 2d plot cannot show multiple curves, cannot accept linestyles
… accept the modifier or the data will indicate a drop action
…ction for these 2d plots is still missing
…ots are presented on the side. side plots are clickable for exchanging position and scaling with the big one.
…difier or a dataset was dropped onto
… for-loops in favor of for-each loops and other styling stuff
…_fitpage_by_id, respectively, to avoid confusion
…ist and reduce redundance
…or for the getter
…p creation of the main window in a designated main method instead.
…r update_dataset method header (number of arguments)
…alculate to dataviewer.update_dataset (changes nothing effectively)
…. Add property and setter
…itpage_index, that subtabs.py is not managing anymore.
… data entries, when the plot button for the same fitpage is hit multiple times
DrPaulSharp
force-pushed
the
plotting_refactor_tabs
branch
from
c985f66 to
5612e33
Compare
Description
This is the draft pull request for the branch that my plotting refactoring progress is saved on. subfolder 'plotting_refactor' contains all the elements that are used for the demo right now.
Starting the demo can be simply done by executing MainWindow.py with a sasview python environment (package dependencies should be the same)
Fixes # (issue/issues)
How Has This Been Tested?
--
Review Checklist:
Documentation (check at least one)
This defenitely needs some documenting, although the code is not documented so well in the current state. I will add this in the future when more comfortable with the state of the demo.
Licencing (untick if necessary)