Add RemoteDandiset.has_data_standard() convenience function #958

bendichter · 2022-04-13T16:59:48Z

@jwodder, what do you think of something like this?

dandi/dandiapi.py

codecov · 2022-04-13T17:07:37Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (75b2a39) 89.04% compared to head (a994e54) 88.55%.
Report is 265 commits behind head on master.

Files	Patch %	Lines
dandi/dandiapi.py	37.50%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #958      +/-   ##
==========================================
- Coverage   89.04%   88.55%   -0.50%     
==========================================
  Files          76       77       +1     
  Lines        9698    10490     +792     
==========================================
+ Hits         8636     9289     +653     
- Misses       1062     1201     +139

Flag	Coverage Δ
unittests	`88.55% <76.19%> (-0.50%)`	⬇️

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.

dandi/dandiapi.py

Co-authored-by: John T. Wodder II <[email protected]>

dandi/dandiapi.py

bendichter · 2022-10-31T18:51:41Z

@yarikoptic before I write tests for this, how does this syntax look?

jwodder · 2022-10-31T18:53:28Z

dandi/dandiapi.py

+        else:
+            raise ValueError(
+                f"'data_standard' must be an RRID (of form 'RRID:XXX_NNNNNNN`) or one "
+                f"of the following values: {DATA_STANDARD_MAP.keys()}"


dict.keys() doesn't stringify in a human-friendly way. Use ", ".join(DATA_STANDARD_MAP.keys()) instead.

jwodder · 2022-10-31T18:55:12Z

dandi/dandiapi.py

-        This is determined by checking for "RRID:SCR_015242" in the "dataStandard" field
-        of the assetsSummary of the dandiset.
+        Returns True if the Dandiset contains one or more files of the indicated
+        standard. Otherwise, returns False.


The docstring should document the accepted data standards (or, at the very least, the RRID input format).

dandi/dandiapi.py

Co-authored-by: Yaroslav Halchenko <[email protected]>

yarikoptic · 2022-11-01T16:43:46Z

dandi/dandiapi.py

+                f"'data_standard' must be an RRID (of form 'RRID:XXX_NNNNNNN`) or one "
+                f"of the following values: {", ".join(DATA_STANDARD_MAP.keys())}"
+            )
+        assets_summary = self.get_raw_metadata()["assetsSummary"]


well, assetsSummary seems to be non optional, so must be present in a valid metadata record, but if not we would get KeyError here... which could be good as to alert user that smth is wrong with metadata in that dandisets. But should it be an exception really or just a warning? I would lean to a be a warning and just return False. WDYT @jwodder and @satra ?

@jwodder and @bendichter WDYT - better be strict or robust?

I'd be fine with changing this to throw a warning if assetsSummary is missing

yarikoptic · 2022-11-01T16:44:30Z

@yarikoptic before I write tests for this, how does this syntax look?

looks ok to me ;) left few insignificant comments

yarikoptic · 2023-02-22T17:40:20Z

@bendichter I have converted PR back to draft since needs more work. some comments are left above, and also would be nice to add some rudimentary unittest. Overall I think it would be a nice addition, so would be great to see if finished.

bendichter · 2023-03-06T21:02:09Z

I could use some guidance in how to write a test for this. Can I just use published dandisets on the archive?

jwodder · 2023-03-07T19:49:57Z

@bendichter Could you rebase or make an empty commit or something in order for the CI to run again?

jwodder · 2023-03-07T19:51:06Z

@bendichter

Can I just use published dandisets on the archive?

That's one option. Normally, I'd suggest just using the nwb_dandiset and text_dandiset fixtures, but that would require the Dockerized server to produce the assetsSummary immediately, and I don't know how long that takes.

# Conflicts: # dandi/tests/test_dandiapi.py

bendichter · 2023-03-08T00:37:54Z

ok, I added some tests. Let me know if these will work

jwodder · 2023-03-08T14:35:22Z

@bendichter Please run pre-commit on your code.

bendichter · 2023-03-10T13:57:56Z

@jwodder oh, I see. Is there a reason to not set the pre-commit hooks as GitHub Actions? We do this for NeuroConv and it works great. All PRs are automatically Black formatted, so there is no need for developers to worry about installing and running pre-commit.

dandi/dandiapi.py

yarikoptic · 2023-11-17T20:38:26Z

I think it is still a legit and potentially useful interface. @bendichter - what do you think? it might benefit from adhering to the same design principle of using Enum's instead of free strings, as RFed into by #1357 yesterday

bendichter · 2023-11-30T12:36:45Z

@yarikoptic what do you mean by Enum? Do you mean using Literal in the typehints?

jwodder · 2023-11-30T13:01:50Z

@bendichter He means using an enum type instead of strings so that Python and mypy will both complain if a name is typoed and passing an unsupported name becomes impossible. Look at 2f38f8e#diff-a0dcf236f5c18d66953b7985098c75c21bbc9dd6b7e6e7063d5d38c2044f6697.

bendichter · 2023-11-30T13:41:49Z

OK, that looks like a fine approach. I'm not able to work on this right now. Feel free to take this over and implement Enums if you want to push this through now, otherwise, I can get to this next week

Co-authored-by: Yaroslav Halchenko <[email protected]>

jwodder · 2023-11-30T14:03:55Z

dandi/dandiapi.py

+        if assets_summary is None:
+            warnings.warn(
+                f"The raw metadata of RemoteDandiset {self.identifier} does not contain 'assetsSummary'. "
+                f"Assuming that it does not contain {data_standard}.")


@bendichter You do not appear to be using pre-commit; if you were, I'm quite certain black would have moved that closing parenthesis to the next line. Please install pre-commit for the repository and run it.

@jwodder in this particular case, I accepted @yarikoptic 's proposed change in GitHub. I'll make sure pre-commit is installed and running properly for future PRs

yarikoptic · 2024-08-19T21:52:01Z

OK, that looks like a fine approach. I'm not able to work on this right now. Feel free to take this over and implement Enums if you want to push this through now, otherwise, I can get to this next week

:letsdoit:! @bendichter -- would you have time/effort to finish it up?

add RemoteDandiset.is_nwb() convenience function

9cd1ea1

bendichter requested a review from jwodder April 13, 2022 16:59

jwodder requested changes Apr 13, 2022

View reviewed changes

dandi/dandiapi.py Outdated Show resolved Hide resolved

dandi/dandiapi.py Outdated Show resolved Hide resolved

bendichter added 2 commits April 13, 2022 17:02

add docstring and remove unnecessary []

27b0ce5

add type annotation

21261b1

bendichter requested a review from jwodder April 13, 2022 21:04

jwodder requested changes Apr 13, 2022

View reviewed changes

dandi/dandiapi.py Outdated Show resolved Hide resolved

Update dandi/dandiapi.py

74b3749

Co-authored-by: John T. Wodder II <[email protected]>

yarikoptic reviewed Apr 15, 2022

View reviewed changes

dandi/dandiapi.py Outdated Show resolved Hide resolved

generalize method to has_data_standard

5bc0f54

jwodder requested changes Oct 31, 2022

View reviewed changes

bendichter commented Nov 1, 2022

View reviewed changes

dandi/dandiapi.py Outdated Show resolved Hide resolved

bendichter added 3 commits October 31, 2022 21:02

Update dandi/dandiapi.py

2b008ef

better docstring

fce70eb

Merge remote-tracking branch 'origin/add/is_nwb' into add/is_nwb

285a2b9

yarikoptic reviewed Nov 1, 2022

View reviewed changes

dandi/dandiapi.py Outdated Show resolved Hide resolved

Update dandi/dandiapi.py

b1aabee

Co-authored-by: Yaroslav Halchenko <[email protected]>

yarikoptic reviewed Nov 1, 2022

View reviewed changes

yarikoptic marked this pull request as draft February 22, 2023 17:39

jwodder changed the title ~~add RemoteDandiset.is_nwb() convenience function~~ add RemoteDandiset. has_data_standard() convenience function Mar 7, 2023

yarikoptic requested a review from jwodder March 7, 2023 19:47

bendichter added 2 commits March 7, 2023 21:34

fix string rendering and add tests

203d4ad

Merge branch 'master' into add/is_nwb

12bd523

# Conflicts: # dandi/tests/test_dandiapi.py

bendichter marked this pull request as ready for review March 8, 2023 00:37

jwodder changed the title ~~add RemoteDandiset. has_data_standard() convenience function~~ Add RemoteDandiset.has_data_standard() convenience function Mar 8, 2023

bendichter added 3 commits March 10, 2023 10:44

Trigger Build

6eea7c4

Trigger Build

d2073c2

flake8

fd5cdd5

rmv space

4908500

bendichter commented Jun 8, 2023

View reviewed changes

dandi/dandiapi.py Outdated Show resolved Hide resolved

Update dandi/dandiapi.py

a994e54

Co-authored-by: Yaroslav Halchenko <[email protected]>

jwodder reviewed Nov 30, 2023

View reviewed changes

yarikoptic marked this pull request as draft October 10, 2024 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RemoteDandiset.has_data_standard() convenience function #958

Add RemoteDandiset.has_data_standard() convenience function #958

bendichter commented Apr 13, 2022

codecov bot commented Apr 13, 2022 •

edited

Loading

bendichter commented Oct 31, 2022

jwodder Oct 31, 2022

jwodder Oct 31, 2022

yarikoptic Nov 1, 2022

yarikoptic Jun 7, 2023

bendichter Jun 8, 2023

yarikoptic commented Nov 1, 2022

yarikoptic commented Feb 22, 2023

bendichter commented Mar 6, 2023 •

edited

Loading

jwodder commented Mar 7, 2023

jwodder commented Mar 7, 2023 •

edited

Loading

bendichter commented Mar 8, 2023

jwodder commented Mar 8, 2023

bendichter commented Mar 10, 2023

yarikoptic commented Nov 17, 2023

bendichter commented Nov 30, 2023

jwodder commented Nov 30, 2023

bendichter commented Nov 30, 2023

jwodder Nov 30, 2023

bendichter Nov 30, 2023

yarikoptic commented Aug 19, 2024

Add RemoteDandiset.has_data_standard() convenience function #958

Are you sure you want to change the base?

Add RemoteDandiset.has_data_standard() convenience function #958

Conversation

bendichter commented Apr 13, 2022

codecov bot commented Apr 13, 2022 • edited Loading

Codecov Report

bendichter commented Oct 31, 2022

jwodder Oct 31, 2022

Choose a reason for hiding this comment

jwodder Oct 31, 2022

Choose a reason for hiding this comment

yarikoptic Nov 1, 2022

Choose a reason for hiding this comment

yarikoptic Jun 7, 2023

Choose a reason for hiding this comment

bendichter Jun 8, 2023

Choose a reason for hiding this comment

yarikoptic commented Nov 1, 2022

yarikoptic commented Feb 22, 2023

bendichter commented Mar 6, 2023 • edited Loading

jwodder commented Mar 7, 2023

jwodder commented Mar 7, 2023 • edited Loading

bendichter commented Mar 8, 2023

jwodder commented Mar 8, 2023

bendichter commented Mar 10, 2023

yarikoptic commented Nov 17, 2023

bendichter commented Nov 30, 2023

jwodder commented Nov 30, 2023

bendichter commented Nov 30, 2023

jwodder Nov 30, 2023

Choose a reason for hiding this comment

bendichter Nov 30, 2023

Choose a reason for hiding this comment

yarikoptic commented Aug 19, 2024

codecov bot commented Apr 13, 2022 •

edited

Loading

bendichter commented Mar 6, 2023 •

edited

Loading

jwodder commented Mar 7, 2023 •

edited

Loading