juaml/junifer
Watch
3
0
Fork
You've already forked junifer
 0
Code Issues 14 Pull requests 2 Packages Activity Actions

fix: storage #103

Merged
synchon merged 83 commits from fix/storage into main 2022-10-17 16:54:37 +00:00
Conversation 17 Commits 83 Files changed 20 +717 -529
synchon commented 2022-10-14 10:42:32 +00:00 (Migrated from github.com)
Copy link

This PR improves the storage interface with a single method on storage-like object intuitively called store(). It also fixes the interaction with the markers and lets the marker choose the kind of storage needed. The storage-like object is responsible for implementing the necessary methods which marker.store() and finally storage.store() will call for example, store_timeseries(), store_table() and store_matrix().

As this was a major refactor for the internals, a lot of other minor changes had to implemented including method name changes, type annotations and docstring upgrades.

* [x] fix #84, #74 * [x] description of feature/fix * [x] tests added/passed * [x] add an entry to the [latest changes](../docs/changes/latest.inc) This PR improves the storage interface with a single method on `storage-like` object intuitively called `store()`. It also fixes the interaction with the markers and lets the marker choose the `kind` of storage needed. The `storage-like` object is responsible for implementing the necessary methods which `marker.store()` and finally `storage.store()` will call for example, `store_timeseries()`, `store_table()` and `store_matrix()`. As this was a major refactor for the internals, a lot of other minor changes had to implemented including method name changes, type annotations and docstring upgrades.
codecov[bot] commented 2022-10-14 10:54:23 +00:00 (Migrated from github.com)
Copy link

Codecov Report

Merging #103 (d4e8990) into main (3133e69) will decrease coverage by 0.04%.
The diff coverage is 93.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
- Coverage   87.88%   87.84%   -0.05%     
==========================================
  Files          49       49              
  Lines        1857     1867      +10     
  Branches      339      342       +3     
==========================================
+ Hits         1632     1640       +8     
- Misses        180      182       +2     
  Partials       45       45
Impacted Files Coverage Δ
junifer/markers/parcel.py 94.91% <75.00%> (ø)
junifer/markers/ets_rss.py 92.30% <83.33%> (+0.30%) ⬆️
junifer/storage/base.py 94.11% <89.47%> (-5.89%) ⬇️
junifer/markers/base.py 91.48% <91.66%> (-8.52%) ⬇️
junifer/markers/__init__.py 100.00% <100.00%> (ø)
junifer/markers/collection.py 100.00% <100.00%> (ø)
junifer/markers/functional_connectivity_atlas.py 100.00% <100.00%> (ø)
junifer/markers/functional_connectivity_spheres.py 100.00% <100.00%> (ø)
junifer/markers/sphere_aggregation.py 97.67% <100.00%> (+4.65%) ⬆️
junifer/storage/sqlite.py 94.24% <100.00%> (+1.42%) ⬆️
... and 3 more
# [Codecov](https://codecov.io/gh/juaml/junifer/pull/103?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report > Merging [#103](https://codecov.io/gh/juaml/junifer/pull/103?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (d4e8990) into [main](https://codecov.io/gh/juaml/junifer/commit/3133e6938e95e7b2e8eeb8e431971d948214d1f9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (3133e69) will **decrease** coverage by `0.04%`. > The diff coverage is `93.61%`. [![Impacted file tree graph](https://codecov.io/gh/juaml/junifer/pull/103/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/103?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #103 +/- ## ========================================== - Coverage 87.88% 87.84% -0.05% ========================================== Files 49 49 Lines 1857 1867 +10 Branches 339 342 +3 ========================================== + Hits 1632 1640 +8 - Misses 180 182 +2 Partials 45 45 ``` | [Impacted Files](https://codecov.io/gh/juaml/junifer/pull/103?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/markers/parcel.py](https://codecov.io/gh/juaml/junifer/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3BhcmNlbC5weQ==) | `94.91% <75.00%> (ø)` | | | [junifer/markers/ets\_rss.py](https://codecov.io/gh/juaml/junifer/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL2V0c19yc3MucHk=) | `92.30% <83.33%> (+0.30%)` | :arrow_up: | | [junifer/storage/base.py](https://codecov.io/gh/juaml/junifer/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9zdG9yYWdlL2Jhc2UucHk=) | `94.11% <89.47%> (-5.89%)` | :arrow_down: | | [junifer/markers/base.py](https://codecov.io/gh/juaml/junifer/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL2Jhc2UucHk=) | `91.48% <91.66%> (-8.52%)` | :arrow_down: | | [junifer/markers/\_\_init\_\_.py](https://codecov.io/gh/juaml/junifer/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL19faW5pdF9fLnB5) | `100.00% <100.00%> (ø)` | | | [junifer/markers/collection.py](https://codecov.io/gh/juaml/junifer/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL2NvbGxlY3Rpb24ucHk=) | `100.00% <100.00%> (ø)` | | | [junifer/markers/functional\_connectivity\_atlas.py](https://codecov.io/gh/juaml/junifer/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL2Z1bmN0aW9uYWxfY29ubmVjdGl2aXR5X2F0bGFzLnB5) | `100.00% <100.00%> (ø)` | | | [junifer/markers/functional\_connectivity\_spheres.py](https://codecov.io/gh/juaml/junifer/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL2Z1bmN0aW9uYWxfY29ubmVjdGl2aXR5X3NwaGVyZXMucHk=) | `100.00% <100.00%> (ø)` | | | [junifer/markers/sphere\_aggregation.py](https://codecov.io/gh/juaml/junifer/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3NwaGVyZV9hZ2dyZWdhdGlvbi5weQ==) | `97.67% <100.00%> (+4.65%)` | :arrow_up: | | [junifer/storage/sqlite.py](https://codecov.io/gh/juaml/junifer/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9zdG9yYWdlL3NxbGl0ZS5weQ==) | `94.24% <100.00%> (+1.42%)` | :arrow_up: | | ... and [3 more](https://codecov.io/gh/juaml/junifer/pull/103/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | |
fraimondo (Migrated from github.com) requested changes 2022-10-14 10:57:12 +00:00
junifer/markers/collection.py
@ -104,8 +119,11 @@ class MarkerCollection:
fraimondo (Migrated from github.com) commented 2022-10-14 10:48:29 +00:00
Copy link

This should still be validate.

There are two methods:

validate_input: checks the input
get_output_kind: gives the output kind (given the input)

validate calls both of them

This should still be validate. There are two methods: `validate_input`: checks the input `get_output_kind`: gives the output kind (given the input) `validate` calls both of them
fraimondo (Migrated from github.com) commented 2022-10-14 10:49:18 +00:00
Copy link

Same here, the method should be validate

Same here, the method should be `validate`
junifer/storage/base.py
fraimondo (Migrated from github.com) commented 2022-10-14 10:53:34 +00:00
Copy link

It still should be the same: validate. However, the storage has no output.

It still should be the same: `validate`. However, the storage has no output.
junifer/storage/base.py
fraimondo (Migrated from github.com) commented 2022-10-14 10:55:40 +00:00
Copy link

This is kind of tricky.

read_features should be able to read any kind of features: tables, matrix, nifti, etc.

The previous name read_df, aimed to tell you which kind of object you were getting.

I'm sill not sure about this change. We might need to discuss it in the channel.

This is kind of tricky. `read_features` should be able to read any kind of features: tables, matrix, nifti, etc. The previous name `read_df`, aimed to tell you which kind of object you were getting. I'm sill not sure about this change. We might need to discuss it in the channel.
synchon (Migrated from github.com) reviewed 2022-10-14 12:21:20 +00:00
junifer/markers/collection.py
@ -104,8 +119,11 @@ class MarkerCollection:
synchon (Migrated from github.com) commented 2022-10-14 12:21:20 +00:00
Copy link

Okay makes sense since it comes from PipelineStepMixin.

Okay makes sense since it comes from `PipelineStepMixin`.
synchon (Migrated from github.com) reviewed 2022-10-14 12:25:39 +00:00
junifer/storage/base.py
synchon (Migrated from github.com) commented 2022-10-14 12:25:39 +00:00
Copy link

So it raises an error if validate fails?

So it raises an error if `validate` fails?
synchon (Migrated from github.com) reviewed 2022-10-14 12:33:38 +00:00
junifer/storage/base.py
synchon (Migrated from github.com) commented 2022-10-14 12:33:38 +00:00
Copy link

From what I see, validate in SQLiteFeatureStorage returns a bool.

From what I see, `validate` in `SQLiteFeatureStorage` returns a bool.
synchon (Migrated from github.com) reviewed 2022-10-14 12:34:12 +00:00
junifer/storage/base.py
synchon (Migrated from github.com) commented 2022-10-14 12:34:11 +00:00
Copy link

I see your point. Does it still remain an abstractmethod?

I see your point. Does it still remain an `abstractmethod`?
fraimondo (Migrated from github.com) reviewed 2022-10-17 08:37:34 +00:00
junifer/storage/base.py
fraimondo (Migrated from github.com) commented 2022-10-17 08:37:34 +00:00
Copy link

yes, at it really depends on the storage.

yes, at it really depends on the storage.
fraimondo (Migrated from github.com) reviewed 2022-10-17 08:38:15 +00:00
junifer/storage/base.py
fraimondo (Migrated from github.com) commented 2022-10-17 08:38:15 +00:00
Copy link

It should not. It should raise an error if it fails to validate the action.

It should not. It should raise an error if it fails to validate the action.
synchon (Migrated from github.com) reviewed 2022-10-17 08:40:33 +00:00
junifer/storage/base.py
synchon (Migrated from github.com) commented 2022-10-17 08:40:32 +00:00
Copy link

What I understand from this is that all storage-like object need to have a way to read into DataFrames, right?

What I understand from this is that all storage-like object need to have a way to read into DataFrames, right?
synchon (Migrated from github.com) reviewed 2022-10-17 08:41:23 +00:00
junifer/storage/base.py
synchon (Migrated from github.com) commented 2022-10-17 08:41:23 +00:00
Copy link

Okay then I'll make the changes.

Okay then I'll make the changes.
synchon (Migrated from github.com) reviewed 2022-10-17 08:54:43 +00:00
junifer/markers/collection.py
@ -104,8 +119,11 @@ class MarkerCollection:
synchon (Migrated from github.com) commented 2022-10-17 08:54:43 +00:00
Copy link

Done.

Done.
synchon commented 2022-10-17 10:57:05 +00:00 (Migrated from github.com)
Copy link

@fraimondo waiting for your approval.

@fraimondo waiting for your approval.
fraimondo commented 2022-10-17 12:59:38 +00:00 (Migrated from github.com)
Copy link

I'll check it now (testing the bot)

I'll check it now (testing the bot)
fraimondo (Migrated from github.com) requested changes 2022-10-17 13:04:24 +00:00
junifer/storage/base.py
fraimondo (Migrated from github.com) commented 2022-10-17 13:01:53 +00:00
Copy link
else:
    raise ValueError(f"I don't know how to store {kind}")
``` else: raise ValueError(f"I don't know how to store {kind}") ```
junifer/storage/sqlite.py
fraimondo (Migrated from github.com) commented 2022-10-17 13:02:50 +00:00
Copy link

Not being tested?

Not being tested?
fraimondo (Migrated from github.com) commented 2022-10-17 13:03:26 +00:00
Copy link

Can we make it more specific here?

I know it will be a lot of logic, but we can actually tell the user which is the kind that we can't store.

Can we make it more specific here? I know it will be a lot of logic, but we can actually tell the user which is the kind that we can't store.
fraimondo (Migrated from github.com) commented 2022-10-17 13:03:55 +00:00
Copy link

Indeed this can go to the "base" class, as the _valid_inputs can be a parameter (like the "on" of markers)

Indeed this can go to the "base" class, as the _valid_inputs can be a parameter (like the "on" of markers)
synchon (Migrated from github.com) reviewed 2022-10-17 13:43:49 +00:00
junifer/storage/base.py
synchon (Migrated from github.com) commented 2022-10-17 13:43:49 +00:00
Copy link

Done.

Done.
synchon (Migrated from github.com) reviewed 2022-10-17 14:38:06 +00:00
junifer/storage/sqlite.py
synchon (Migrated from github.com) commented 2022-10-17 14:38:06 +00:00
Copy link

Done.

Done.
fraimondo (Migrated from github.com) approved these changes 2022-10-17 16:47:55 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fraimondo
Labels
Clear labels   
CRITICAL
High Priority Issue

  
Stale

   
WIP
Work in progress

  
bug
Something isn't working

  
concept
Design/Implementation concepts

  
coordinate
Issues or pull requests related to coordinates (ROIs)

  
dataset
Issues or pull requests related to datagrabbers or datasets

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
maintenance
Issues or pull requests related to maintenance

  
maps
Issues or pull requests related to maps (probabilistic atlases)

  
marker
Issues or pull requests related to markers

  
mask
Issues or pull requests related to masks

  
on hold
Issue waiting on other issue(s)

  
parcellation
Issues or pull requests related to parcellations (deterministic atlases)

  
preprocess
Issues or pull requests related to preprocessors

  
question
Further information is requested

  
ready
Pull request is ready for review and merge

  
storage
Issues or pull requests related to marker storage or retrieval

  
template-space
Issues or pull requests related to template spaces

  
triage
New issues waiting to be reviewed

  
wontfix
This will not be worked on

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
Milestone
Clear milestone
No items
No milestone
0.0.1 (alpha 0)
Assignees
Clear assignees
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!103
No description provided.