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

GeoLocationNameValue with examples #2308

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

turbomam
Copy link
Member

@turbomam turbomam commented Dec 19, 2024

I sneaked in a few white-space auto-reformats

@turbomam turbomam requested review from cmungall and aclum December 19, 2024 19:02
@turbomam turbomam linked an issue Dec 19, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Dec 19, 2024

PR Preview Action v1.5.0
🚀 Deployed preview to https://microbiomedata.github.io/nmdc-schema/pr-preview/pr-2308/
on branch gh-pages at 2025-01-09 18:04 UTC

@turbomam
Copy link
Member Author

turbomam commented Dec 19, 2024

I wanted to use see_alsos for the INSDC and CIA links for country_or_sea's links but got

ValueError: https://www.cia.gov/the-world-factbook/ and http://unstats.un.org/unsd/methods/m49/m49.htm is not a valid URI or CURIE

@aclum
Copy link
Contributor

aclum commented Jan 6, 2025

This is acceptable to me but needs coordination with code that generate biosample records @sujaypatil96 @pkalita-lbl @corilo
we would also need a migrator for existing data FYI @eecavanna @brynnz22

Comment on lines +46 to +47
description: A geo-political description of a region that is a direct, unambiguous subdivision of a paired country_or_sea value,
OR an unambiguous region or feature within an ocean or "sea".
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @turbomam, it's not clear to me what the two operands of the OR are here.

Interpretation 1 (I added EITHER to mark the first operand):

...a region that is "EITHER" a [...] subdivision [...] OR an unambiguous...

Interpretation 2 (I added "EITHER" to mark the first operand):

...a region that is a [...] subdivision of EITHER a [...] value, OR an unambiguous...

Comment on lines +46 to +47
description: A geo-political description of a region that is a direct, unambiguous subdivision of a paired country_or_sea value,
OR an unambiguous region or feature within an ocean or "sea".
Copy link
Collaborator

Choose a reason for hiding this comment

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

A few lines above this, we have the word sea without quotes.

MIxS states that country or sea names...

I don't know why it is enclosed in quotes here.

...within an ocean or "sea".

@@ -15,11 +15,31 @@ classes:
class_uri: 'nmdc:AttributeValue'
description: >-
The value for any value of a attribute for a sample. This object can hold both the un-normalized atomic
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated to this PR (I'm commenting on it while it's in my attention): I don't understand this sentence.

The value for any value of a attribute for a sample.

@eecavanna
Copy link
Collaborator

we would also need a migrator for existing data FYI

Thanks for the heads up. In this PR, so far, I am not seeing any schema changes that would make existing data invalid. Maybe those schema changes are coming soon (or I'm overlooking them here).

@aclum
Copy link
Contributor

aclum commented Jan 8, 2025

also need input from kitware folks since this would impact nmdc-server code. @jeffbaumes @naglepuff @marySalvi

@turbomam
Copy link
Member Author

turbomam commented Jan 9, 2025

@eecavanna the need for the migration is based on the fact that the range of geo_loc_name will change from string to an instance of this class.

@eecavanna
Copy link
Collaborator

Thanks, @turbomam. That makes sense to me. I assume that changing the range of geo_log_name will happen via a separate PR, since I don't see it in the diff of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create a GeoLocationNameValue class with substructure
3 participants