Allow to postprocess on read-only waveform folders #1957
Conversation
alejoe91
added
core
alejoe91
changed the title
alejoe91
changed the title
There was a problem hiding this comment.
This seems like an important feature to support.
I have some comments about nesting that come from clomatic complexityh which is related to code complexity / hader-to-read more bugs:
https://en.wikipedia.org/wiki/Cyclomatic_complexity
This is why I try to decrease this when I see it.
As a more general comment I wonder if there is another way of accomplishing (supporting operating in read only waveforms) without having to keep state and sacrifice caching.
Could we just store the extension folder somewhere else instead of the folder where we usually do? After all, that is like or own in-house format so we can do this right? Like, we can use the local caching of the user (temp) and save that as state so the user can still benefit from caching and we keep the state as the folder of the extensions instead of a flag to see if the original folder of the waveforms is read only.
Maybe there are obvious downsides to this that I had not considered. Just throwing out some other possible solutions.
| @@ -399,6 +403,9 @@ def return_scaled(self) -> bool: | |||
| def dtype(self): | |||
| return self._params["dtype"] | |||
|
|
|||
| def is_read_only(self) -> bool: | |||
There was a problem hiding this comment.
Another comment.
An advice that I have read is that logical / boolean flags should be designed in such a way that most of the testing you do will test for True.
Reason being, that putting not in front of flags is a little bit harder to read as you need to keep two things in working memory instead of one (like remembering the sign of an expression that people usually mess up in algebraic high school operations).
Here most of the tests are for not is_read_only. On the other hand, "is_read_only" is the common category / flag that the OS usually uses so that seem more important here.
Just a comment.
There was a problem hiding this comment.
We could do is_not_read_only instead! That's a good point!
There was a problem hiding this comment.
@h-mayorquin I simplified the if-else statements!
src/spikeinterface/postprocessing/tests/common_extension_tests.py
Outdated
Show resolved
Hide resolved
|
@h-mayorquin @zm711 I applied the suggested changes! |
| # Extensions already loaded in memory | ||
| if extension_name in self._loaded_extensions: | ||
| return True | ||
| else: |
There was a problem hiding this comment.
You don't need this else. This will allow you to get rid of the nesting.
| @@ -517,15 +524,19 @@ def is_extension(self, extension_name) -> bool: | |||
| if self.folder is None: | |||
| return extension_name in self._loaded_extensions | |||
| else: | |||
There was a problem hiding this comment.
You don't need this else either. This will allow you to get rid of the nesting.
| @@ -76,6 +77,16 @@ def setUp(self): | |||
| overwrite=True, | |||
| ) | |||
| self.we2 = we2 | |||
|
|
|||
| # make we read-only | |||
| if platform.system() != "Windows": | |||
There was a problem hiding this comment.
Does the test works in windows if this statements is not run? I am wondering if what should be skipped is the test itself if we rely chmod. As @zm711 has pointed out that this is not reliable on windows.
There was a problem hiding this comment.
Altought the core tests are running on windows so...
There was a problem hiding this comment.
My point is not that it won't work just that it may introduce a hard to detect bug in the future since it does not behave as the user intends. So if you ever have a test that address writability, Windows behavior would diverge from Linux/Mac. If all the tests are only testing the readability, then they wouldn't "experience" the difference in the file identities. So that's why I said future problem since it may in the future add a hard to detect bug, but won't definitely introduce one.
|
@h-mayorquin updated with the suggestion. @samuelgarcia only waiting for your review |
In some cases a waveform extractor folder can be loaded in read-only mode (e.g., precomputed waverorms on the cloud, attached as data).
The waveform extensions always inherit the
formatfrom the parentWaveformExtractor. However, if the parent is read-only all postprocessing crashes because it tries to add folders to the parent folder.This PR adds the
is_read_only()method to theWaveformExtractor. In case a waveform extractor is read-only, extensions can be computed, but they will be in memory and not persistent to disk.EDIT:
Added a small fix to load the
similarityextension in theexport_to_phyif that's already available!