Closed SylvainTakerkart closed 1 year ago
this needs a bit more of testing... indeed, my [full-scale] test [on real data] for which the code is not included in this PR shows unexpected behavior of the little changes introduced in this PR!
therefore, this PR is not ready for reviewing!
todo...
Info on the unit tests of BEP032Generator and what I've done so far:
session_folder
actually designates the ephys
folder where the data files live) nor the session.tsv metadata files;ice
or ece
values (which decide whether there is a session-level directory or not), all the existing tests should pass whether we use ephys_type=ece
(old behavior, with session-level directory) or ephys_type=ice
(new behavior for patchclamp data, without session-level directory); in practice, I tried it, and it's indeed the case;ephys_type
attribute, which is either ice
or ece
); I think we need to discuss further what we want to do (larger refactoring, etc.) before it's necessary to implement something in this direction...@JuliaSprenger , if you're ok with this symbolic change in the unit tests, this PR is now ready for review!
Hi @SylvainTakerkart
It looks like the existence or absence of the session folder level is the only difference between the ece
and the ice
case so far.
Are you sure this is the main conceptual difference between these two data types? In the way the current specifications are written also ece
datasets can omit the session level folder and it might as well be possible for ice
datasets to require a session level. Wouldn't it be better to have the optional session level generation independent of the ece
/ice
mode? I.e. make the session name argument optional and omit the session level folder if no value is provided?
Hi @JuliaSprenger, I see what you mean...
The question is whether we want this code to be i) an implementation of the specs with bijective correspondances between the specs and the code, or ii) something more practical, with "recommended implementations" (for which we could e.g recommend to have the session level for ece
and not for ice
). For the base class BEP032Data, we clearly have to go with i), so I agree with you!
A consequence of this is that almost everything becomes "optional": the session directory, as well as everything in the data filenames exept 'sub-' ('ses-', 'run-', 'task-' are optional)... So we might have to remove a good portion of what's coded in the base class' methods... I'm going to look at this! (all this is not clear in my mind anyhow ;) )... If you have hints, I welcome them ;)
With this latest commit, in fact, ephys_type
is not used anywhere... That means in fact that there might not be any "conceptual differences" between ice
and ece
, but only different usages of the same concepts... WDYT? If you're OK, I'll make ephys_type
disappear totally...
Yes, that looks like the better approach to me :+1:
I removed the ephys_type class attribute... I left the two test classes for ece and ice, this might be useful for the future, we'll see... Ready for merging?
Temporary solution, as discussed before the summer: we add an if statement to differentiate extra- and intra-cellular ephys. This decides whether there's a 'session' level in the BIDS hierarchy or not...