Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
9209302 to
349d007
Compare
ma595
left a comment
There was a problem hiding this comment.
Hi Isaac, this is easier to follow, thanks.
Just a few points:
- Can you check if all the imports are used, i.e.
plt,os? - Do our linting checks actually check these files? If not, can you please add this.
- Is there an option to enable you to plot more components for visualisation purposes in the first two rows of the first figure?
- There seems to be a rogue blank plot in the
plot_rmse_depth_profilefunction.. - We definitely need more comments in the Jupyter notebook to explain what's going
on. I also don't really understand what some of the plots are showing. In particular cell 24 and beyond. This can be left to a separate PR. - There's some french in the plots i.e.
train serie - Also the plot dashed lines do not seem to run up to 30 inclusive for the training data. Is this correct?
The commit functionises the plotting code in the Jumper notebook into reusable functions located in a new file, plotting_utils.py. Fix french typo in plot label. The plotting logic remains unchanged.
349d007 to
43aeb95
Compare
This stops excluding the Jumper notebook and restart files (now removed from repo) from linting and fixes linting issues. Adds jupyter files types to pre-commit ruff hook so that notebooks are formatted.
43aeb95 to
98dd222
Compare
Those are no longer needed after the refactor, will remove them.
Not currently, the functions in general are not fully parametrized. I have mostly kept them as is, bar functionising them for now. Plan to do this in step 3 (sub-issue #95) of issue #94
Yes, will fixed
Overall we are replacing this single notebook with a set of example notebooks, this will be done in step 5 of issue #94. I created a larger issue for this to track the work. It will make it easier to see where this PR fits in the overall plan.
Yes, will add the 's'
Yes, it goes from 0 to 29 rather than 1 to 30, so 30 values in total. |
ma595
left a comment
There was a problem hiding this comment.
Thanks for the fixes. Ready to merge.
This PR is to functionise the plotting code which is in the
Jumper.ipynbnotebook:Closes #93