Experiment single dataset #58
Conversation
henrikjacobsenfys
added
[priority] medium
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #58 +/- ##
===========================================
+ Coverage 96.60% 96.93% +0.33%
===========================================
Files 12 14 +2
Lines 412 490 +78
===========================================
+ Hits 398 475 +77
- Misses 14 15 +1
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:
|
| filename = f"{self.name}.h5" | ||
|
|
||
| if not isinstance(filename, str): | ||
| raise TypeError(f"Filename must be a string, not {type(filename).__name__}") |
There was a problem hiding this comment.
calling experiment.save_hdf5() with no arguments (or any filename without a directory component) raises FileNotFoundError because os.path.dirname('test.h5')returns '', so os.makedirs('', exist_ok=True) fails on Windows. The default filename = f"{self.name}.h5" therefore breaks out of the box.
Guard the directory creation with something like
dir_name = os.path.dirname(filename);
if dir_name:
os.makedirs(dir_name, exist_ok=True)| self.name = name | ||
|
|
||
| # TODO: Add checks of dimensions etc. I'm not yet sure what dimensions I want to allow, so for now I trust myself. | ||
| self._data = sc_load_hdf5(filename) |
There was a problem hiding this comment.
Yes, this needs to be guarded a bit better. This assumes (and promises) that sc_load_hdf5 will always return sc.DataArray. We might want to test it at least.
There was a problem hiding this comment.
Added a simple test. The class will see significant improvements to functionality and safety once I start using it with analysis, for now I just want a small proof of concept.
| self._data = value | ||
|
|
||
| def plot_data(self): | ||
| """Plot the dataset using plopp.""" |
There was a problem hiding this comment.
I have no idea if and how to write unit tests for plotting.
use mocking like there's no tomorrow ;)
e.g.
def test_plot_data_success(self, experiment):
"Test plotting data successfully when in notebook environment"
# GIVEN
with patch.object(Experiment, '_in_notebook', return_value=True), \
patch('plopp.plot') as mock_plot, \
patch('IPython.display.display') as mock_display:
mock_fig = MagicMock()
mock_plot.return_value = mock_fig
# WHEN
experiment.plot_data()
# THEN
mock_plot.assert_called_once()
args, kwargs = mock_plot.call_args
assert sc.identical(args[0], experiment._data.transpose())
assert kwargs['title'] == f"{experiment.name}"
mock_display.assert_called_once_with(mock_fig)
def test_plot_data_no_data_raises(self):
"Test plotting data raises ValueError when no data is present"
# GIVEN
experiment = Experiment(name="empty_experiment")
# WHEN / THEN
with pytest.raises(ValueError, match="No data to plot"):
experiment.plot_data()
def test_plot_data_not_in_notebook_raises(self, experiment):
"Test plotting data raises RuntimeError when not in notebook environment"
# GIVEN
with patch.object(Experiment, '_in_notebook', return_value=False):
# WHEN / THEN
with pytest.raises(RuntimeError, match="plot_data\\(\\) can only be used in a Jupyter notebook environment"):
experiment.plot_data()There was a problem hiding this comment.
Thanks! I'll look into it
|
Updated to add tests. |
It seems simpler to have a single data set rather than a collection for the Experiment class. This PR does that.
It's missing methods to bin and play with the data. They can be accessed by working directly on the stored scipp data, and I think that's good enough for now.
It's also missing some checks on the data, but until I have a firmer grasp on what the data will look like, I'll assume it's OK. Things will fail very quickly if the data don't have the right dimensions anyway.
Finally, I'm unsure what to do with the plotting. It works in Jupyter Notebooks, which is all I care about right now, but for the future it may need some improvements. I have no idea if and how to write unit tests for plotting.