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

add "none" to slot analysis/data type #169

Closed
wants to merge 1 commit into from

Conversation

bmeluch
Copy link
Contributor

@bmeluch bmeluch commented Jan 4, 2024

From #166, added "none" to AnalysisTypeEnum

@bmeluch bmeluch requested a review from pkalita-lbl January 4, 2024 23:11
@bmeluch bmeluch linked an issue Jan 4, 2024 that may be closed by this pull request
@pkalita-lbl
Copy link
Collaborator

Sorry for not looking at issue #166 sooner. I just read through it and it's not clear to me if this change needs to be here or in nmdc-schema. Hold tight until we get an update in the issue.

@turbomam
Copy link
Member

turbomam commented Jan 5, 2024

What's the case in which the analysis_type would be 'none'?

Adding 'none' to a list of categorical values is very controversial. Some terminological resources do it extensively (like NCIT, I think) but I don't think we have done this in the nmdc-schema yet and I would prefer not to start.

The slot is recommended, not required in nmdc-schema, but it appears to be required in the submission-schema.

@pkalita-lbl
Copy link
Collaborator

What's the case in which the analysis_type would be 'none'?

It's outlined in the linked issue #166

@@ -6,6 +6,7 @@ AnalysisTypeEnum metagenomics placeholder PV descr
AnalysisTypeEnum metaproteomics placeholder PV descr
AnalysisTypeEnum metatranscriptomics placeholder PV descr
AnalysisTypeEnum natural organic matter placeholder PV descr
AnalysisTypeEnum none placeholder PV descr
Copy link
Contributor

Choose a reason for hiding this comment

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

@bmeluch
Change 'none' to "bulk chemistry"

@mslarae13
Copy link
Contributor

mslarae13 commented Mar 25, 2024

@pkalita-lbl should this change also be made in NMDC schema?

I think the answer is yes, though I don't think we track this in nmdc-main. So maybe it's not important for data but would be good for consistancy.
Would like @pkalita-lbl , @turbomam to comment

@bmeluch
See https://github.com/microbiomedata/nmdc-schema/blob/f2690e865b41893cc8dbd519434052087d48e559/src/schema/portal/sample_id.yaml#L24

@pkalita-lbl
Copy link
Collaborator

pkalita-lbl commented Mar 25, 2024

should this change also be made in NMDC schema?

It should only be made in nmdc-schema. The fact that submission-schema was redefining AnalysisTypeEnum was a mistake that I fixed recently (which is also why this PR has merge conflicts).

The path forward should be:

  • Close this PR
  • Make the change in nmdc-schema (Montana has correctly pointed out where to make the change)
  • When a new version of nmdc-schema has been released with that change, update sheets_and_friends/tsv_in/import_slots_regardless.tsv in this repo to import the new version

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.

add "bulk chemistry" to slot analysis/data type
4 participants