-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extract file names #14
Conversation
What there is in this PR?This PR deals with the part of the codebase dedicated to loading the data. The feature here implemented makes it possible to get the file names to be loaded. There are two ways to load the data:
DescriptionThe loading module creates a
For recursive search I am using Pre saved folder pathsThe pre saved folder paths are three:
Their specificity is declined in the parser File object
|
load_suite2p/config/config.yml
Outdated
|
||
server: 'ssh.swc.ucl.ac.uk' | ||
|
||
paths: | ||
winstor: '/Volumes/your_server/' | ||
imaging: '/Volumes/path/to/imaging/data/' | ||
winstor: '/Volumes/winstor/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General point, but I guess we don't want these paths hard coded in the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I could add it to .gitignore
to just stop tracking it.
load_suite2p/load/load_data.py
Outdated
|
||
config_path = Path(__file__).parent / "config/config.yml" | ||
config_path = Path(__file__).parents[1] / "config/config.yml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, (eventually) the configs should live somewhere else and not be tracked by git.
load_suite2p/config/config.yml
Outdated
winstor: '/Volumes/winstor/' | ||
imaging: '/Volumes/winstor/swc/margrie/Chryssanthi/imaging' | ||
allen-dff: '/Volumes/winstor/swc/margrie/Chryssanthi/imaging/allen_dff' | ||
serial2p: '/Volumes/winstor/swc/margrie/Chryssanthi/imaging/serial2p' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand why serial2p is being processed by this repo? Isn't this tool just for functional imaging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Serial2p" here refers to a folder in which serial2p output is saved and that is then loaded. imaging
, allen-dff
, serial2p
, stimulus-ai-schedule
are names of folders from which I might want to load the data.
load_suite2p/objects/file.py
Outdated
self.path: Path = path | ||
self._path_str = str(path) | ||
self.datatype = self._get_data_type() | ||
self.analysistype = self._get_analysis_type() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(very) minor point, but for consistency and readability, I'd call this self.analysis_type
load_suite2p/objects/file.py
Outdated
elif "allen_dff.mat" in self._path_str: | ||
return self.DataType.ALLEN_DFF | ||
else: | ||
raise ValueError("File not to be used") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be worth logging these type of things at some point as well as throwing an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I log it in a very summarized form in a function that instantiates File
. I could track the path of all discarded files, the only downside is that they would be quite a lot and the logging file would be enormous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's raises an error, presumably it shouldn't be raised often?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the exception as a way to block the instantiation of a File obj and count the number of discarded files. But well, maybe it's improper. I can think of a better way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Giving a quick look online, it seems that there is a cleaner way to do it with __new__
pass | ||
|
||
@abstractmethod | ||
def get_path_to_stimulus_AI_schedule_files(self) -> Path: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure it's obvious, but I wouldn't use abbreviations (e.g. AI
) if it wouldn't be totally clear what this means to anyone reading the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll double check the precise meaning of AI in this case and make it more explicit
Looks good :) |
No description provided.