-
Notifications
You must be signed in to change notification settings - Fork 0
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
Multiple updates supporting ROs 4542, 4552, 4553, & 4555 #17
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @brindakv! One comment
"type": "array", | ||
"items": { | ||
"type": "object", | ||
"properties": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the pdbx_database_accession
is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this up @valasatava.
This has been addressed in the latest commit. Please review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @brindakv. Just one doubt about bringing in DOIs
} | ||
] | ||
}, | ||
"pdbx_DOI": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to load the DOI as well? I think we're currently building DOIs on the fly in frontend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @valasatava.
It is an attribute in the public database_2
category. Is it ok to have it in the data warehouse for external users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valasatava do you recommend that we suppress pdbx_DOI
explicitly? Or can it be included as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no issue with including this item, but I am being cautious about what we expose in the public API. @josemduarte what's your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can't hurt to include it. So that we offer the data out to users. But let's not add to search.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @josemduarte. There is no search metadata for pdbx_DOI
.
This PR addresses the following:
database_2
to support extended PDB IDs5.410
pdbx_vrpt_summary_geometry
rcsb_polymer_instance_feature_summary.coverage