-
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
Created Class ChemicalConversionProcess
#5
Conversation
…ata/berkeley-schema-fy24 into make-ReactionProcess
Fixed a typo
src/schema/nmdc.yaml
Outdated
@@ -256,6 +256,28 @@ classes: | |||
- has_solution_components | |||
- volume | |||
|
|||
ChemicalReagent: |
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.
we already have modeling for this. I'll post it in a minute
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.
Resolution: make ChemicalReagent specific to the fact that it is used in a laboratory. like "LaboratoryChemicalReagent" or "LaboratoryReagent"
src/schema/nmdc.yaml
Outdated
@@ -1650,6 +1672,26 @@ classes: | |||
syntax: "{id_nmdc_prefix}:cspro-{id_shoulder}-{id_blade}{id_version}{id_locus}" | |||
interpolated: true | |||
|
|||
ReactionProcess: |
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.
please give this a name that explicitly differentiates it from the exiting ReactionClass
(which is about biological pathways)
I suggest LaboratoryReactionPathway
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.
https://microbiomedata.github.io/nmdc-schema/Reaction/
I think ReactionProcess is as far as we can get this right now. I could see arguments for changing Reaction too.
Let's not dwell on this term and track the issue as a todo
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.
Is class ReactionProcess
not differentiated enough from class Reaction
?
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.
Resolution: come up with a name descriptive to the fact that the process takes place in an experimental setting rather than being functional annotation
@turbomam We cannot combine the LaboratoryReagent class to be an input because we need to let |
description: The reagents used in a chemical reaction. | ||
multivalued: true | ||
inlined_as_list: true | ||
sample_state_information: |
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.
add Domain
@JamesTessmer @anastasiyaprymolenna can you add what migration is needed here... add this to the 1st comment, and update it accordingly ( I think it's limited to no migration, but not 100% sure) Collections requiring migration:
|
Will do, we were talking about that earlier today. I’m planning to update the metabolink/material procssing PRs we worked on with the needed info. |
ChemicalConversionProcess
This PR only created a new class and slots and shouldn't require a data migration.