Fix/bug_53 #123
No reviewers
Labels
No labels
CRITICAL
Stale
WIP
bug
concept
coordinate
dataset
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
maintenance
maps
marker
mask
on hold
parcellation
preprocess
question
ready
storage
template-space
triage
wontfix
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
juaml/junifer!123
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/bug_53"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Codecov Report
100.00% <ø> (ø)92.10% <95.00%> (+0.01%)Flags with carried forward coverage won't be shown. Click here to find out more.
100.00% <ø> (ø)100.00% <ø> (ø)97.72% <90.90%> (-2.28%)95.04% <92.30%> (-4.96%)100.00% <100.00%> (ø)100.00% <100.00%> (+2.27%)100.00% <100.00%> (ø)97.75% <100.00%> (-0.05%)100.00% <100.00%> (ø)70.83% <100.00%> (+0.94%)@ -83,6 +83,8 @@ EnhancementsInclude PR number?
@ -139,6 +130,24 @@ class BaseDataGrabber(ABC):"""A more explicit description:
For each item in the ``element`` tuple passed to ``__getitem__()``, this method returns the corresponding key(s).@ -155,3 +164,24 @@ class BaseDataGrabber(ABC):msg="Concrete classes need to implement get_elements().",element : dict... -> Dict[str, Path]:?@ -90,0 +107,4 @@self.uri, path=tmpdir,clone_options=["-n", "--depth=1"])repo.checkout(name=".datalad/config", options=["HEAD"])remote_id = repo.config.get("datalad.dataset.id", None)Maybe raise as error here?
Maybe use a new variable here instead of re-assigning.
I think it should be changed to a
NotImplementedErrorwith the message:get_item() is not useful for this class, hence not implemented.@ -58,0 +82,4 @@This function is not implemented for this class as it is useless."""raise NotImplementedError("get_item() is not useful for this class, hence not implemented.")... -> Dict[str, Path]:?A more explicit description:
For each item in the ``element`` tuple passed to ``__getitem__()``, this method returns the corresponding key(s).list of str@ -107,3 +107,3 @@def _replace_patterns_glob(self, element: Tuple, pattern: str) -> str:def _replace_patterns_glob(self, element: Dict, pattern: str) -> str:"""Replace patterns with the element so it can be globbed.The type for
elementin the docstring needs to be updated.@ -121,27 +121,39 @@ class PatternDataGrabber(BaseDataGrabber):The pattern with the element replaced.list of strThe description can be updated:
This method constructs a real path to an item by replacing the ``patterns`` with actual values passed via ``**element`` and then returns the path.@ -143,2 +154,2 @@each item in the tuple is the value for the replacement stringspecified in "replacements".element : dictThe element to be indexed. The keys must be the same as theI think the description here can be rephrased like so:
This method returns the ``replacements`` of patterns defined for the datagrabber.out = {}@ -155,3 +165,3 @@element = (element,)out = {}for t_type in self.types:t_pattern = self.patterns[t_type]... -> Dict[str, Path]:?@ -24,13 +24,24 @@ class OasisVBMTestingDatagrabber(BaseDataGrabber):types = ["VBM_GM"]list of str... -> Dict[str, Path]:?@ -82,6 +92,17 @@ class SPMAuditoryTestingDatagrabber(BaseDataGrabber):types = ["BOLD", "T1w"] # TODO: Check that they are T1wlist of str@ -93,13 +114,13 @@ class SPMAuditoryTestingDatagrabber(BaseDataGrabber):"""... -> Dict[str, Path]:?@ -176,6 +194,17 @@ class PartlyCloudyTestingDataGrabber(BaseDataGrabber):)list of str@ -187,13 +216,13 @@ class PartlyCloudyTestingDataGrabber(BaseDataGrabber):"""... -> Dict[str, Path]:?No. It's the Data Object, so Dict[str, Dict[str, Any]]
@ -90,0 +107,4 @@self.uri, path=tmpdir,clone_options=["-n", "--depth=1"])repo.checkout(name=".datalad/config", options=["HEAD"])remote_id = repo.config.get("datalad.dataset.id", None)No need, this is just because mypy can't understand that
_dataset.repois not None at this point.@ -58,0 +82,4 @@This function is not implemented for this class as it is useless."""raise NotImplementedError("get_item() is not useful for this class, hence not implemented.")no, it's the Data Object.
@ -90,0 +107,4 @@self.uri, path=tmpdir,clone_options=["-n", "--depth=1"])repo.checkout(name=".datalad/config", options=["HEAD"])remote_id = repo.config.get("datalad.dataset.id", None)I'm a bit unsure of adding an
assertstatement just because ofmypy.@ -90,0 +107,4 @@self.uri, path=tmpdir,clone_options=["-n", "--depth=1"])repo.checkout(name=".datalad/config", options=["HEAD"])remote_id = repo.config.get("datalad.dataset.id", None)well, it's also good to double check
@ -90,0 +107,4 @@self.uri, path=tmpdir,clone_options=["-n", "--depth=1"])repo.checkout(name=".datalad/config", options=["HEAD"])remote_id = repo.config.get("datalad.dataset.id", None)Yeah that's why I thought an error would be better for this case.
@ -11,2 +27,4 @@def test_datalad_base_abstractness() -> None:"""Test datalad base is abstract."""Needs
Parameterssection.@ -81,12 +82,37 @@ class DataladDataGrabber(BaseDataGrabber):logger.debug(f"\t_rootdir = {rootdir}")For consistency:
... dataset IDFor consistency:
... dataset ID from ...@ -90,3 +115,4 @@def _dataset_get(self, out: Dict) -> Dict:"""Get the dataset found from the path in `out`.Isn't there any other way apart from cloning the dataset again? I'm just trying to understand.
@ -98,39 +124,90 @@ class DataladDataGrabber(BaseDataGrabber):ReturnsCan a platform-neutral representation be used here for
t_path?Ditto.
... different ID.def cleanup(self) -> None:Maybe a platform-neutral representation for
fhere?@ -134,3 +210,4 @@self._dataset.drop(f, result_renderer="disabled")def __getitem__(self, element: Union[str, Tuple]) -> Dict[str, Path]:"""Implement single element indexing in the Datalad database.What if
t_out["path"]is already aPath?@ -134,3 +210,4 @@self._dataset.drop(f, result_renderer="disabled")def __getitem__(self, element: Union[str, Tuple]) -> Dict[str, Path]:"""Implement single element indexing in the Datalad database.It's not. It comes from datalad.
@ -98,39 +124,90 @@ class DataladDataGrabber(BaseDataGrabber):ReturnsWhat do you mean?
@ -134,3 +210,4 @@self._dataset.drop(f, result_renderer="disabled")def __getitem__(self, element: Union[str, Tuple]) -> Dict[str, Path]:"""Implement single element indexing in the Datalad database.I see, okay.
@ -98,39 +124,90 @@ class DataladDataGrabber(BaseDataGrabber):ReturnsI mean something like:
str(t_path.absolute())ort_path.resolve()@ -90,3 +115,4 @@def _dataset_get(self, out: Dict) -> Dict:"""Get the dataset found from the path in `out`.No. You need to get the
.datalad/configfile in the remote. The roblem is that the remote could have different protocols (https, ssh, etc). So doing a no-checkout (-n) shallow (--depth=1) clone and then just getting only the required file is the fastest. It's done only one, oninstall, only if the dataset was already cloned.@ -90,3 +115,4 @@def _dataset_get(self, out: Dict) -> Dict:"""Get the dataset found from the path in `out`.Okay thanks!
🚀