Experiment mwp #90
Experiment mwp #90
Conversation
henrikjacobsenfys
added
[scope] enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #90 +/- ##
==========================================
Coverage ? 97.63%
==========================================
Files ? 30
Lines ? 1606
Branches ? 268
==========================================
Hits ? 1568
Misses ? 21
Partials ? 17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AndrewSazonov
left a comment
AndrewSazonov
left a comment
There was a problem hiding this comment.
Small suggestion: it would be nice to update the placeholder markdown header cells to use the notebook title and short description I added in the previous PR (and also add one to the new experiment.ipynb). That way the code cells can automatically stretch to the full page width.
| value = int(value) | ||
| # This line can be removed when scipp resize support | ||
| # resizing with coordinates | ||
| dimensions[dim] = value |
There was a problem hiding this comment.
You're modifying dimensions in-place, while iterating over its items. This can lead to BAD things. Consider creating a copy of dimensions before and iterating over the copy (or the other way around)
| """Get the Q values from the dataset.""" | ||
| if self._data is None: | ||
| warnings.warn('No data loaded.', UserWarning) | ||
| return None |
There was a problem hiding this comment.
None is not the advertised sc.Variable. Consider changing -> sc.Variable to sc.Variable | None
| if self._data is None: | ||
| raise ValueError('No data to save.') | ||
|
|
||
| import os |
There was a problem hiding this comment.
Top of the file or top of the function? I believe I put it here to only do the import if we're actually saving data, so after input checking :)
Edit: put it at the top of the file
| 'giving the Experiment an invalid name' | ||
| # WHEN / THEN EXPECT | ||
| with pytest.raises(TypeError): | ||
| experiment.load_hdf5('some_file.h5', name=123) |
There was a problem hiding this comment.
Shouldn't the argument be display_name?
There was a problem hiding this comment.
It should indeed, I wonder how it passed the test
docs/docs/tutorials/index.md
Outdated
| a model of diffusion | ||
| - [Sample model](sample_model.ipynb) – Learn how to create a model of | ||
| the scattering from your sample | ||
| - [Experiment] (experiment.ipyng) - Learn how to load and bin your data |
There was a problem hiding this comment.
This should read experiment.ipynb
There was a problem hiding this comment.
Why can't I react with the 'see no evil' emoji?
| import plopp as pp | ||
| import scipp as sc | ||
|
|
||
| # from easyscience.job.experiment import ExperimentBase |
| ########### | ||
|
|
||
| @staticmethod | ||
| def _in_notebook(): |
There was a problem hiding this comment.
this method should probably be typed to return -> bool
There was a problem hiding this comment.
Fixed this and another missing type hint :)
Co-authored-by: Andrew Sazonov <AndrewSazonov@users.noreply.github.com>
Good point, will do this! |
Add experiment class and minimal functionality