Review June 2020 #26
Review June 2020 #26
Conversation
Codecov Report
@@ Coverage Diff @@
## review/project_review_june_2020 #26 +/- ##
==================================================================
Coverage ? 90.61%
==================================================================
Files ? 37
Lines ? 6444
Branches ? 0
==================================================================
Hits ? 5839
Misses ? 605
Partials ? 0 Continue to review full report at Codecov.
|
nvaytet
left a comment
nvaytet
left a comment
There was a problem hiding this comment.
In summary, there is not much wrong with the plotting code itself, but there is a lot of duplication.
I strongly recommend to make it so that make_sub_plot and make_animation are just calling make_plot.
I would also try to split this in multiple files to make it easier to navigate. You don't have many files in that directory, so it should be fine to so that (i know that when you start getting many many files, it also starts to make things difficult to follow, but you are not there yet).
mcstasscript/interface/plotter.py
Outdated
|
|
||
| index = -1 | ||
| for data in data_list: | ||
| index = index + 1 |
There was a problem hiding this comment.
index doesn't appear to be used in this loop
There was a problem hiding this comment.
Did a larger refactoring of the plotting functionality, this silly issue was removed. I can't respond directly to the top comment / summary, but the plotting code was refactored to remove the code duplication. The new version has been pushed to the master branch. The three classes are now all functions, which depend on a simpler underlying function. This also allowed easier and better testing!
mcstasscript/interface/plotter.py
Outdated
|
|
||
| print("Plotting data with name " + data.metadata.component_name) | ||
| if type(data.metadata.dimension) == int: | ||
| fig = plt.figure(0) |
There was a problem hiding this comment.
i recommend to get the axes as an object here, as the get current axes method gca is always prone to break, since some other axes somewhere else may become the current active axes.
| fig = plt.figure(0) | |
| fig, ax0 = plt.subplots() |
this way you are always sure which axes you are dealing with
There was a problem hiding this comment.
and maybe give the option to change the figsize? plt.subplots(figsize=(12, 8)) (width and height in inches)
should be passed from input arguments?
There was a problem hiding this comment.
This change was included in the refactor, the internal plotter function now uses fig and ax in the arguments which is provided by the calling function. In that way plt.gca is never used, and the ax object is used for all plotting.
figsize is now also a keyword argument in all three plotting functions.
mcstasscript/interface/plotter.py
Outdated
| y_err = data.Error | ||
|
|
||
| #(fig, ax0) = plt.errorbar(x, y, yerr=y_err) | ||
| plt.errorbar(x, y, yerr=y_err) |
There was a problem hiding this comment.
Then call the methods on the axes, to also be sure they are plotting on the correct axes
| plt.errorbar(x, y, yerr=y_err) | |
| ax0.errorbar(x, y, yerr=y_err) |
There was a problem hiding this comment.
All plotting is now done through the ax object instead of plt directly. Great suggestion.
mcstasscript/interface/plotter.py
Outdated
| #(fig, ax0) = plt.errorbar(x, y, yerr=y_err) | ||
| plt.errorbar(x, y, yerr=y_err) | ||
|
|
||
| ax0 = plt.gca() |
There was a problem hiding this comment.
| ax0 = plt.gca() |
There was a problem hiding this comment.
Avoided by giving the internal plotting function the ax as an argument.
mcstasscript/interface/plotter.py
Outdated
| data.metadata.limits[1]*x_axis_mult) | ||
|
|
||
| # Add a title | ||
| plt.title(data.metadata.title) |
There was a problem hiding this comment.
| plt.title(data.metadata.title) | |
| ax0.set_title(data.metadata.title) |
mcstasscript/interface/plotter.py
Outdated
| plt.show() | ||
|
|
||
|
|
||
| class make_sub_plot: |
There was a problem hiding this comment.
I think the recommended convention is to have uppercase letters in class names, and underscores in function names.
e.g. MakeSubPlot.
There was a problem hiding this comment.
That said, this class only has an __init__ and no other functions associated to it.
Maybe it can just be a function? Does it need to be a class? Is another class inheriting from it somehere?
If not, I would just make it a function, if it is just making a figure. Matplotlib will take care of keeping the figure alive for you, it doesn't need to be in a class. Especially since I don't really see the use of self anywhere in here.
There was a problem hiding this comment.
make_plot, make_sub_plot and make_animation are now all functions!
mcstasscript/interface/plotter.py
Outdated
| plt.show() | ||
|
|
||
|
|
||
| class make_animation: |
There was a problem hiding this comment.
In this case this might be useful to have a class, see below
mcstasscript/interface/plotter.py
Outdated
| # initialize plots | ||
|
|
||
| data = data_list[0] | ||
| if isinstance(data.metadata.dimension, int): |
There was a problem hiding this comment.
Again i would try to just call make_plot here instead of duplicating again.
There was a problem hiding this comment.
Made a internal plotting function instead that make_plot, make_sub_plot and make_animation all use. The amount of duplication was really bad, glad to be rid of it!
mcstasscript/interface/plotter.py
Outdated
|
|
||
| def init_1D(): | ||
| # initialize function for animation | ||
| er.set_data([], [], []) # wont work |
There was a problem hiding this comment.
I would be very careful using functions inside function to access the scope of the __init__. I would either
- pass things as arguments (e.g.
erneeds to be an input argument and this function needs to be outside of the class) - or use the fact that we are inside a class, and make
era member of the class (self.er = ...) and then make these functions class members, i.e.
def init_1D(self):
self.er = set_data([], [], [])There was a problem hiding this comment.
The FuncAnimation need a function for updating the data with only an index as argument (unless I am misunderstanding), and in such I need to define a short function inside this function. I find it acceptable since the function is so short.
mcstasscript/interface/plotter.py
Outdated
|
|
||
| class make_sub_plot: | ||
| """ | ||
| make_plot plots contents of McStasData objects |
There was a problem hiding this comment.
| make_plot plots contents of McStasData objects | |
| make_sub_plot plots contents of McStasData objects |
copied and pasted from above?
There was a problem hiding this comment.
Added additional text, but make_plot and make_sub_plot are still close in their description, as the functions are rather similar.
nvaytet
left a comment
nvaytet
left a comment
There was a problem hiding this comment.
I can see that you have tests for the plot options, but can't really see tests on the plotting functions.
I know testing plotting is incredibly difficult, but maybe a first step would be to just call the plotting functions with different parameters, and simply make sure they don't throw any errors.
It would be a start.
|
|
||
| def test_McStasPlotOptions_default_orders_of_mag(self): | ||
| """ | ||
| Test that newly created McStasMetaData has correct type |
There was a problem hiding this comment.
These docstrings all the same for all tests. Should be updated.
There was a problem hiding this comment.
I have expanded the testing of McStasPlotOptions (only some options were tested), and updated their doc strings.
In addition a new file was added, test_Plotter.py, which adds 600 lines of testing for the plotting functionality, including running the internal plotter function (without plt.show() ) for all 1D/2D and log mode combinations.
This could obviously still be improved in the sense that actual plotting is not tested or visually compared to a known correct plot.
This improves readability but also consistency accros platforms, as the order dependent on which order the folders were read by the system.
between linux, os x and windows
find . on os x
CI with GitHub actions
Fix bad escape sequence warning in python 3.11
… instead of near DEFINE and parameters. This syntax works where the others have issues.
Fixing SEARCH system
Before the nexus file would be opened and closed for each monitor to allow using the same code when either one or all monitors were loaded. This may have caused issues on VISA. Now the file is opened once and all data read when the user wants all monitors, and it is handdled differently when just one monitor is loaded, leading to slighlty more complex code, but hopefully fix the issue with mulitple readers on VISA.
Change to how NeXus data is read.
Will function from notebooks, provided McStas 3.5.35 is used.
Let McStasScript default to use of webgl-classic visualization.
…o fill in meaningful information.
Fix typos in docs and tutorials, Thanks Milan!
Increase compatibility with mcdoc util
Update string format representation used in instrument file output
Added default value to _read_configuration so a line_length is always available. Added pypi publish ci
As this project has not been properly reviewed, it is necessary to make a complete review in order to assure quality.
Due to the size of the task, it is broken down to smaller tasks which are described on Jira to manage the work. This pull request will be used for adding review comments and improvements along the way without affecting the master branch.