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

Convert DOI URLs in related_publications to related resources #1417

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Mar 7, 2024

Closes #1308.

@jwodder jwodder added minor Increment the minor version when merged NWB cmd-upload labels Mar 7, 2024
Copy link

codecov bot commented Mar 7, 2024

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 88.61%. Comparing base (ee18a15) to head (b0bf065).

Files Patch % Lines
dandi/metadata/util.py 82.35% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1417      +/-   ##
==========================================
+ Coverage   88.54%   88.61%   +0.06%     
==========================================
  Files          77       77              
  Lines       10537    10554      +17     
==========================================
+ Hits         9330     9352      +22     
+ Misses       1207     1202       -5     
Flag Coverage Δ
unittests 88.61% <82.35%> (+0.06%) ⬆️

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.

@bendichter
Copy link
Contributor

LGTM!

@jwodder
Copy link
Member Author

jwodder commented Mar 7, 2024

@bendichter Then add an approving review for your review request.

@bendichter
Copy link
Contributor

It's currently marked as WIP

@jwodder jwodder marked this pull request as ready for review March 7, 2024 15:09
@jwodder
Copy link
Member Author

jwodder commented Mar 7, 2024

@bendichter How about now?

@jwodder
Copy link
Member Author

jwodder commented Mar 11, 2024

@yarikoptic Ping.

related.append(
models.Resource(
identifier=v,
relation=models.RelationType.IsDescribedBy,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one is is too specific to assume that any reference is the description of the data we find in the file.
What we can say only that the file references that publication, and hence I would better go with

Suggested change
relation=models.RelationType.IsDescribedBy,
relation=models.RelationType.References,

as the default. WDYT @bendichter @satra ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point that we cannot necessarily infer the relationship between the data and the paper.

The DataCite definition for "References" is "indicates B is used as a source of information for A." Here, this would mean that the paper is used as a source of information for the Dandiset. I don't know if this really fits here.

I have been wondering what the best way to associate papers with datasets using these relations. What are the different types of ways a paper and a dataset can be associated? So far, I have been using isDescribedBy (indicates that A describes B) for everything, assuming that the publication is the primary publication in which the data is introduced and described. Another type of relationship might be that a paper reuses a dataset. In this case, IsSupplementTo (indicates that A is a supplement to B ) might be a better choice, since it applies to both. Are there other relationship types that describe how a paper and dandiset might be related? What are the different use-cases here?

This is challenging in part because there is currently no aspect of the dandi schema that allows us to describe what the resource is (see issue here), so that needs to be inferred at least somewhat by the relation type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bendichter Now that dandi/dandi-schema#231 has been resolved, how can we move this PR forwards?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I got it right we still have not made our minds on the default type of the relationship here... The most logical is that if we do not know relationship - do not fill it in. But ATM our model seems to not allow relation to not be present . Should we change that in the model? or you still feel comfortable @bendichter in using IsDescribedBy here ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. My preference would be to change to schema to make this optional. Now that we have a resource type I am much less reliant on this particular field, though I do appreciate the caveat that optional fields are very rarely populated. I would also be happy with defaulting to IsDescribedBy or IsSupplementTo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, to expedite, let's proceed with IsDescribedBy and see where it takes us ;-)

@yarikoptic yarikoptic merged commit d5e2a76 into master Mar 25, 2024
27 of 28 checks passed
@yarikoptic yarikoptic deleted the gh-1308 branch March 25, 2024 18:13
Copy link

github-actions bot commented May 3, 2024

🚀 PR was released in 0.62.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd-upload minor Increment the minor version when merged NWB released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

publications in asset metadata
3 participants