Fix/bug_53 #123

Merged
fraimondo merged 13 commits from fix/bug_53 into main 2022-11-10 12:46:27 +00:00
fraimondo commented 2022-11-07 14:50:08 +00:00 (Migrated from github.com)
  • fix #53
  • description of feature/fix
  • tests added/passed
  • add an entry to the latest changes
* [x] fix #53 * [x] description of feature/fix * [x] tests added/passed * [x] add an entry to the [latest changes](../docs/changes/latest.inc)
codecov[bot] commented 2022-11-07 14:51:37 +00:00 (Migrated from github.com)

Codecov Report

Merging #123 (e238e61) into main (43ba5d4) will increase coverage by 0.01%.
The diff coverage is 95.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #123      +/-   ##
==========================================
+ Coverage   92.08%   92.10%   +0.01%     
==========================================
  Files          55       55              
  Lines        2123     2191      +68     
  Branches      398      414      +16     
==========================================
+ Hits         1955     2018      +63     
- Misses        133      135       +2     
- Partials       35       38       +3     
Flag Coverage Δ
docs 100.00% <ø> (ø)
junifer 92.10% <95.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
junifer/datagrabber/aomic/id1000.py 100.00% <ø> (ø)
junifer/datagrabber/aomic/piop2.py 100.00% <ø> (ø)
junifer/datagrabber/multiple.py 97.72% <90.90%> (-2.28%) ⬇️
junifer/datagrabber/datalad_base.py 95.04% <92.30%> (-4.96%) ⬇️
junifer/datagrabber/aomic/piop1.py 100.00% <100.00%> (ø)
junifer/datagrabber/base.py 100.00% <100.00%> (+2.27%) ⬆️
junifer/datagrabber/hcp.py 100.00% <100.00%> (ø)
junifer/datagrabber/pattern.py 97.75% <100.00%> (-0.05%) ⬇️
junifer/testing/datagrabbers.py 100.00% <100.00%> (ø)
junifer/utils/logging.py 70.83% <100.00%> (+0.94%) ⬆️
# [Codecov](https://codecov.io/gh/juaml/junifer/pull/123?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report > Merging [#123](https://codecov.io/gh/juaml/junifer/pull/123?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (e238e61) into [main](https://codecov.io/gh/juaml/junifer/commit/43ba5d41d929591f8f2fedd01e8e78a49ecd36f7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (43ba5d4) will **increase** coverage by `0.01%`. > The diff coverage is `95.00%`. [![Impacted file tree graph](https://codecov.io/gh/juaml/junifer/pull/123/graphs/tree.svg?width=650&height=150&src=pr&token=5H21JuZXMw&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml)](https://codecov.io/gh/juaml/junifer/pull/123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #123 +/- ## ========================================== + Coverage 92.08% 92.10% +0.01% ========================================== Files 55 55 Lines 2123 2191 +68 Branches 398 414 +16 ========================================== + Hits 1955 2018 +63 - Misses 133 135 +2 - Partials 35 38 +3 ``` | Flag | Coverage Δ | | |---|---|---| | docs | `100.00% <ø> (ø)` | | | junifer | `92.10% <95.00%> (+0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/juaml/junifer/pull/123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/datagrabber/aomic/id1000.py](https://codecov.io/gh/juaml/junifer/pull/123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9hb21pYy9pZDEwMDAucHk=) | `100.00% <ø> (ø)` | | | [junifer/datagrabber/aomic/piop2.py](https://codecov.io/gh/juaml/junifer/pull/123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9hb21pYy9waW9wMi5weQ==) | `100.00% <ø> (ø)` | | | [junifer/datagrabber/multiple.py](https://codecov.io/gh/juaml/junifer/pull/123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9tdWx0aXBsZS5weQ==) | `97.72% <90.90%> (-2.28%)` | :arrow_down: | | [junifer/datagrabber/datalad\_base.py](https://codecov.io/gh/juaml/junifer/pull/123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9kYXRhbGFkX2Jhc2UucHk=) | `95.04% <92.30%> (-4.96%)` | :arrow_down: | | [junifer/datagrabber/aomic/piop1.py](https://codecov.io/gh/juaml/junifer/pull/123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9hb21pYy9waW9wMS5weQ==) | `100.00% <100.00%> (ø)` | | | [junifer/datagrabber/base.py](https://codecov.io/gh/juaml/junifer/pull/123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9iYXNlLnB5) | `100.00% <100.00%> (+2.27%)` | :arrow_up: | | [junifer/datagrabber/hcp.py](https://codecov.io/gh/juaml/junifer/pull/123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9oY3AucHk=) | `100.00% <100.00%> (ø)` | | | [junifer/datagrabber/pattern.py](https://codecov.io/gh/juaml/junifer/pull/123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9wYXR0ZXJuLnB5) | `97.75% <100.00%> (-0.05%)` | :arrow_down: | | [junifer/testing/datagrabbers.py](https://codecov.io/gh/juaml/junifer/pull/123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci90ZXN0aW5nL2RhdGFncmFiYmVycy5weQ==) | `100.00% <100.00%> (ø)` | | | [junifer/utils/logging.py](https://codecov.io/gh/juaml/junifer/pull/123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci91dGlscy9sb2dnaW5nLnB5) | `70.83% <100.00%> (+0.94%)` | :arrow_up: |
github-actions[bot] commented 2022-11-07 15:41:08 +00:00 (Migrated from github.com)
PR Preview Action v1.2.0
Preview removed because the pull request was closed.
2022-11-10 12:50 UTC
[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.2.0 :---: Preview removed because the pull request was closed. 2022-11-10 12:50 UTC <!-- Sticky Pull Request Commentpr-preview -->
synchon (Migrated from github.com) requested changes 2022-11-08 07:28:13 +00:00
@ -83,6 +83,8 @@ Enhancements
synchon (Migrated from github.com) commented 2022-11-07 16:50:33 +00:00

Include PR number?

Include PR number?
@ -139,6 +130,24 @@ class BaseDataGrabber(ABC):
"""
synchon (Migrated from github.com) commented 2022-11-08 06:18:05 +00:00

A more explicit description: For each item in the ``element`` tuple passed to ``__getitem__()``, this method returns the corresponding key(s).

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().",
synchon (Migrated from github.com) commented 2022-11-08 05:50:06 +00:00

element : dict

`element : dict`
synchon (Migrated from github.com) commented 2022-11-08 06:43:49 +00:00

... -> Dict[str, Path]:?

`... -> 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)
synchon (Migrated from github.com) commented 2022-11-08 07:16:53 +00:00

Maybe raise as error here?

Maybe raise as error here?
synchon (Migrated from github.com) commented 2022-11-08 07:18:12 +00:00

Maybe use a new variable here instead of re-assigning.

Maybe use a new variable here instead of re-assigning.
synchon (Migrated from github.com) commented 2022-11-08 06:49:27 +00:00

I think it should be changed to a NotImplementedError with the message: get_item() is not useful for this class, hence not implemented.

I think it should be changed to a `NotImplementedError` with 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.")
synchon (Migrated from github.com) commented 2022-11-08 06:47:02 +00:00

... -> Dict[str, Path]:?

`... -> Dict[str, Path]:`?
synchon (Migrated from github.com) commented 2022-11-08 06:50:11 +00:00

A more explicit description: For each item in the ``element`` tuple passed to ``__getitem__()``, this method returns the corresponding key(s).

A more explicit description: ```For each item in the ``element`` tuple passed to ``__getitem__()``, this method returns the corresponding key(s).```
synchon (Migrated from github.com) commented 2022-11-08 06:50:25 +00:00

list of str

`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.
synchon (Migrated from github.com) commented 2022-11-08 06:01:27 +00:00

The type for element in the docstring needs to be updated.

The type for `element` in the docstring needs to be updated.
@ -121,27 +121,39 @@ class PatternDataGrabber(BaseDataGrabber):
The pattern with the element replaced.
synchon (Migrated from github.com) commented 2022-11-08 06:02:32 +00:00

list of str

`list of str`
synchon (Migrated from github.com) commented 2022-11-08 06:31:10 +00:00

The 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.

The 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 string
specified in "replacements".
element : dict
The element to be indexed. The keys must be the same as the
synchon (Migrated from github.com) commented 2022-11-08 06:06:00 +00:00

I think the description here can be rephrased like so: This method returns the ``replacements`` of patterns defined for the datagrabber.

I think the description here can be rephrased like so: `This method returns the ``replacements`` of patterns defined for the datagrabber.`
synchon (Migrated from github.com) commented 2022-11-08 06:31:29 +00:00

out = {}

`out = {}`
@ -155,3 +165,3 @@
element = (element,)
out = {}
for t_type in self.types:
t_pattern = self.patterns[t_type]
synchon (Migrated from github.com) commented 2022-11-08 06:44:17 +00:00

... -> Dict[str, Path]:?

`... -> Dict[str, Path]:`?
@ -24,13 +24,24 @@ class OasisVBMTestingDatagrabber(BaseDataGrabber):
types = ["VBM_GM"]
synchon (Migrated from github.com) commented 2022-11-08 07:24:12 +00:00

list of str

`list of str`
synchon (Migrated from github.com) commented 2022-11-08 07:25:52 +00:00

... -> Dict[str, Path]:?

`... -> Dict[str, Path]:`?
@ -82,6 +92,17 @@ class SPMAuditoryTestingDatagrabber(BaseDataGrabber):
types = ["BOLD", "T1w"] # TODO: Check that they are T1w
synchon (Migrated from github.com) commented 2022-11-08 07:26:14 +00:00

list of str

`list of str`
@ -93,13 +114,13 @@ class SPMAuditoryTestingDatagrabber(BaseDataGrabber):
"""
synchon (Migrated from github.com) commented 2022-11-08 07:26:34 +00:00

... -> Dict[str, Path]:?

`... -> Dict[str, Path]:`?
@ -176,6 +194,17 @@ class PartlyCloudyTestingDataGrabber(BaseDataGrabber):
)
synchon (Migrated from github.com) commented 2022-11-08 07:27:05 +00:00

list of str

`list of str`
@ -187,13 +216,13 @@ class PartlyCloudyTestingDataGrabber(BaseDataGrabber):
"""
synchon (Migrated from github.com) commented 2022-11-08 07:27:32 +00:00

... -> Dict[str, Path]:?

`... -> Dict[str, Path]:`?
fraimondo (Migrated from github.com) reviewed 2022-11-08 09:36:25 +00:00
fraimondo (Migrated from github.com) commented 2022-11-08 09:36:24 +00:00

No. It's the Data Object, so Dict[str, Dict[str, Any]]

No. It's the Data Object, so Dict[str, Dict[str, Any]]
fraimondo (Migrated from github.com) reviewed 2022-11-08 09:38:53 +00:00
@ -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)
fraimondo (Migrated from github.com) commented 2022-11-08 09:38:51 +00:00

No need, this is just because mypy can't understand that _dataset.repo is not None at this point.

No need, this is just because mypy can't understand that `_dataset.repo` is not None at this point.
fraimondo (Migrated from github.com) reviewed 2022-11-08 09:40:25 +00:00
@ -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.")
fraimondo (Migrated from github.com) commented 2022-11-08 09:40:14 +00:00

no, it's the Data Object.

no, it's the Data Object.
synchon (Migrated from github.com) reviewed 2022-11-08 09:42:38 +00:00
@ -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)
synchon (Migrated from github.com) commented 2022-11-08 09:42:34 +00:00

I'm a bit unsure of adding an assert statement just because of mypy.

I'm a bit unsure of adding an `assert` statement just because of `mypy`.
fraimondo (Migrated from github.com) reviewed 2022-11-08 10:25:26 +00:00
@ -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)
fraimondo (Migrated from github.com) commented 2022-11-08 10:25:26 +00:00

well, it's also good to double check

well, it's also good to double check
synchon (Migrated from github.com) reviewed 2022-11-08 12:09:04 +00:00
@ -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)
synchon (Migrated from github.com) commented 2022-11-08 12:09:04 +00:00

Yeah that's why I thought an error would be better for this case.

Yeah that's why I thought an error would be better for this case.
synchon (Migrated from github.com) reviewed 2022-11-08 12:12:39 +00:00
@ -11,2 +27,4 @@
def test_datalad_base_abstractness() -> None:
"""Test datalad base is abstract."""
synchon (Migrated from github.com) commented 2022-11-08 12:12:38 +00:00

Needs Parameters section.

Needs `Parameters` section.
synchon (Migrated from github.com) reviewed 2022-11-10 09:14:50 +00:00
@ -81,12 +82,37 @@ class DataladDataGrabber(BaseDataGrabber):
logger.debug(f"\t_rootdir = {rootdir}")
synchon (Migrated from github.com) commented 2022-11-10 09:01:17 +00:00

For consistency: ... dataset ID

For consistency: `... dataset ID`
synchon (Migrated from github.com) commented 2022-11-10 09:01:41 +00:00

For consistency: ... dataset ID from ...

For consistency: `... dataset ID from ...`
@ -90,3 +115,4 @@
def _dataset_get(self, out: Dict) -> Dict:
"""Get the dataset found from the path in `out`.
synchon (Migrated from github.com) commented 2022-11-10 08:57:28 +00:00

Isn't there any other way apart from cloning the dataset again? I'm just trying to understand.

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):
Returns
synchon (Migrated from github.com) commented 2022-11-10 08:59:58 +00:00

Can a platform-neutral representation be used here for t_path?

Can a platform-neutral representation be used here for `t_path`?
synchon (Migrated from github.com) commented 2022-11-10 09:00:04 +00:00

Ditto.

Ditto.
synchon (Migrated from github.com) commented 2022-11-10 09:00:26 +00:00

... different ID.

`... different ID.`
synchon (Migrated from github.com) commented 2022-11-10 09:03:12 +00:00

def cleanup(self) -> None:

`def cleanup(self) -> None:`
synchon (Migrated from github.com) commented 2022-11-10 09:03:49 +00:00

Maybe a platform-neutral representation for f here?

Maybe a platform-neutral representation for `f` here?
@ -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.
synchon (Migrated from github.com) commented 2022-11-10 08:58:50 +00:00

What if t_out["path"] is already a Path?

What if `t_out["path"]` is already a `Path`?
fraimondo (Migrated from github.com) reviewed 2022-11-10 10:09:29 +00:00
@ -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.
fraimondo (Migrated from github.com) commented 2022-11-10 10:09:26 +00:00

It's not. It comes from datalad.

It's not. It comes from datalad.
fraimondo (Migrated from github.com) reviewed 2022-11-10 10:09:34 +00:00
@ -98,39 +124,90 @@ class DataladDataGrabber(BaseDataGrabber):
Returns
fraimondo (Migrated from github.com) commented 2022-11-10 10:09:33 +00:00

What do you mean?

What do you mean?
synchon (Migrated from github.com) reviewed 2022-11-10 10:10:45 +00:00
@ -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.
synchon (Migrated from github.com) commented 2022-11-10 10:10:45 +00:00

I see, okay.

I see, okay.
synchon (Migrated from github.com) reviewed 2022-11-10 10:11:30 +00:00
@ -98,39 +124,90 @@ class DataladDataGrabber(BaseDataGrabber):
Returns
synchon (Migrated from github.com) commented 2022-11-10 10:11:30 +00:00

I mean something like: str(t_path.absolute()) or t_path.resolve()

I mean something like: `str(t_path.absolute())` or `t_path.resolve()`
fraimondo (Migrated from github.com) reviewed 2022-11-10 10:12:57 +00:00
@ -90,3 +115,4 @@
def _dataset_get(self, out: Dict) -> Dict:
"""Get the dataset found from the path in `out`.
fraimondo (Migrated from github.com) commented 2022-11-10 10:12:56 +00:00

No. You need to get the .datalad/config file 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, on install, only if the dataset was already cloned.

No. You need to get the `.datalad/config` file 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, on `install`, only if the dataset was already cloned.
synchon (Migrated from github.com) reviewed 2022-11-10 10:15:27 +00:00
@ -90,3 +115,4 @@
def _dataset_get(self, out: Dict) -> Dict:
"""Get the dataset found from the path in `out`.
synchon (Migrated from github.com) commented 2022-11-10 10:15:27 +00:00

Okay thanks!

Okay thanks!
synchon (Migrated from github.com) approved these changes 2022-11-10 12:23:50 +00:00
synchon (Migrated from github.com) left a comment

🚀

🚀
Sign in to join this conversation.
No reviewers
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
juaml/junifer!123
No description provided.