Skip to content
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

Built-in BIDS support for dandi upload #1011

Merged
merged 30 commits into from
Jul 8, 2022
Merged

Built-in BIDS support for dandi upload #1011

merged 30 commits into from
Jul 8, 2022

Conversation

TheChymera
Copy link
Contributor

@TheChymera TheChymera commented May 12, 2022

@yarikoptic @satra still as draft, some things might still be weird, and need to come up with tests.

Eventually it will look like this (contains both normal and validation ignore usage), and if the number of invalid files is not 18k, the output also does not need to contain 18k lines :)
https://ppb.chymera.eu/1c32cb

@codecov
Copy link

codecov bot commented May 12, 2022

Codecov Report

Attention: Patch coverage is 88.39286% with 13 lines in your changes missing coverage. Please review.

Project coverage is 88.41%. Comparing base (14030cb) to head (8139966).
Report is 1117 commits behind head on master.

Files Patch % Lines
dandi/upload.py 84.21% 6 Missing ⚠️
dandi/bids_validator_xs.py 37.50% 5 Missing ⚠️
dandi/bids_utils.py 92.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1011      +/-   ##
==========================================
+ Coverage   87.65%   88.41%   +0.75%     
==========================================
  Files          65       72       +7     
  Lines        8189     9233    +1044     
==========================================
+ Hits         7178     8163     +985     
- Misses       1011     1070      +59     
Flag Coverage Δ
unittests 88.41% <88.39%> (+0.75%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lgtm-com
Copy link

lgtm-com bot commented May 12, 2022

This pull request introduces 1 alert when merging 46bcdb1 into 14030cb - view on LGTM.com

new alerts:

  • 1 for Use of the return value of a procedure

@TheChymera TheChymera added enhancement New feature or request minor Increment the minor version when merged BIDS cmd-upload labels May 12, 2022
@lgtm-com
Copy link

lgtm-com bot commented May 13, 2022

This pull request introduces 1 alert when merging d508c7b into 0016e60 - view on LGTM.com

new alerts:

  • 1 for Use of the return value of a procedure

@TheChymera TheChymera marked this pull request as ready for review May 16, 2022 09:55
@yarikoptic yarikoptic requested a review from jwodder June 3, 2022 03:03
dandi/upload.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
dandi/validate.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
dandi/validate.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
@TheChymera TheChymera marked this pull request as draft June 7, 2022 11:26
@TheChymera
Copy link
Contributor Author

@jwodder putting it on draft for now to not waste your time. Need to test some things re typing first.

@TheChymera TheChymera marked this pull request as ready for review June 8, 2022 07:48
@TheChymera
Copy link
Contributor Author

@jwodder ok, figured out the typing issues :) Everything else you mentioned should already be in.

dandi/upload.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
dandi/validate.py Outdated Show resolved Hide resolved
@TheChymera TheChymera requested a review from jwodder June 9, 2022 09:37
dandi/upload.py Show resolved Hide resolved
dandi/validate.py Outdated Show resolved Hide resolved
dandi/upload.py Outdated Show resolved Hide resolved
@TheChymera
Copy link
Contributor Author

TheChymera commented Jul 6, 2022

@yarikoptic @jwodder should be done, unless you notice some additional issues. Not sure what's going on with the documentation, but probably unrelated.

dandi/tests/fixtures.py Outdated Show resolved Hide resolved
@TheChymera
Copy link
Contributor Author

@jwodder @yarikoptic could I get another review on this, or otherwise could this be merged?

@yarikoptic
Copy link
Member

I will check it out tomorrow morning. Thanks!

@yarikoptic
Copy link
Member

I have reservations since testing is quite "limited" as for number of use cases code tries to cover with the discovery of bids datasets... also tried on some dummy ds210 from bids-examples, it still required explicit paths and --allow-any-path... will file separate issues for that. For now -- let's just proceed.

@github-actions
Copy link

🚀 PR was released in 0.45.0 🚀

@jwodder jwodder mentioned this pull request Jul 19, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BIDS cmd-upload enhancement New feature or request minor Increment the minor version when merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants