Analyzer set recording #2785
Analyzer set recording #2785
Conversation
|
I really like this idea because we often do the analysis and then move everything over to our server for long-term storage which means the recording-analyzer link gets broken. My only concern (and this is now from experience helping others) is that people forget what goes with what. So is there any way we can have a safety check? This is honestly the end-users responsibility not to mess up, but are there any attributes on the analyzer that could say help make sure the recording being temporarily used is reasonable? |
I think that we could check that the recording has the correct recording attributes, i.e., channel_ids, locations, etc |
|
@zm711 can you check this again after the last commit? |
| recording.get_channel_locations(), self.get_channel_locations() | ||
| ), "Recording channel locations do not match." | ||
| if self._recording is not None: | ||
| warnings.warn("SortingAnalyzer recording is already set. This will overwrite the current recording.") |
There was a problem hiding this comment.
| warnings.warn("SortingAnalyzer recording is already set. This will overwrite the current recording.") | |
| warnings.warn("SortingAnalyzer recording is already set. Current recording is being overwritten.") |
I switched from future tense to present tense to try to emphasize it is happening. Future tense gives it a vibe that one could prevent this from happening but by the warning it is too late.
Also maybe do we want to say that if save_as it used this new recording will be written to the metadata so use caution?
There was a problem hiding this comment.
Actually reading both our warnings they sound a little frightening. We are not overwriting the recording, but we are point to a new recording. Maybe we think a bit more how to say this without being scary.
There was a problem hiding this comment.
I think that sounds better. I'm still curious what would happen if I do:
analyzer.set_temp_recording(my_new_recording)
analyzer.save_as('memory')
do stuff
analyzer.save_as('zarr', xx)
Will that then write the temp_recording to the file instead?
There was a problem hiding this comment.
Yes, that's a really good point. Maybe keeping a flag would be good then
…analyzer-set-recording
| recording2_attributes = deepcopy(recording2_attributes) | ||
| if skip_properties: | ||
| recording1_attributes.pop("properties") | ||
| recording2_attributes.pop("properties") |
There was a problem hiding this comment.
Are upi handling the case skip_properties:=false after this ?
I would remove skip_properties option because we do not use.
No ?
| @pytest.fixture(scope="module") | ||
| def get_dataset(): | ||
| return _get_dataset() |
There was a problem hiding this comment.
| @pytest.fixture(scope="module") | |
| def get_dataset(): | |
| return _get_dataset() | |
| @pytest.fixture(scope="module") | |
| def dataset(): | |
| return get_dataset() |
| @@ -15,7 +15,7 @@ | |||
| import numpy as np | |||
|
|
|||
|
|
|||
| def get_dataset(): | |||
| def _get_dataset(): | |||
There was a problem hiding this comment.
| def _get_dataset(): | |
| def get_dataset(): |
There was a problem hiding this comment.
This smater no ?
why private ?
| return _get_dataset() | ||
|
|
||
|
|
||
| def test_SortingAnalyzer_memory(tmp_path, get_dataset): |
There was a problem hiding this comment.
| def test_SortingAnalyzer_memory(tmp_path, get_dataset): | |
| def test_SortingAnalyzer_memory(tmp_path, dataset): |
There was a problem hiding this comment.
The get_dataset sounds like a function but here it the variable.
| test_SortingAnalyzer_memory(tmp_path) | ||
| test_SortingAnalyzer_binary_folder(tmp_path) | ||
| test_SortingAnalyzer_zarr(tmp_path) | ||
| dataset = _get_dataset() |
There was a problem hiding this comment.
| dataset = _get_dataset() | |
| dataset = get_dataset() |
This is useful for example if the analyzer was set up and ran with a tmp cached recording, but later on one wants to set a lazy recording to perform additional operations and computations