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

Deprecate EnvironmentalMaterialTerm, Pathway #222

Merged
merged 6 commits into from
Aug 7, 2024

Conversation

bmeluch
Copy link

@bmeluch bmeluch commented Jul 9, 2024

See microbiomedata#1881

This PR is the first step in deprecating EnvironmentalMaterialTerm, FunctionalAnnotationTerm + children. It adds a deprecated: string to each class definition. This PR will need to be reviewed to see if/when it can be merged in during the Berkeley schema rollout, or if we need to wait.

Copy link

github-actions bot commented Jul 9, 2024

PR Preview Action v1.4.7
🚀 Deployed preview to https://microbiomedata.github.io/berkeley-schema-fy24/pr-preview/pr-222/
on branch gh-pages at 2024-08-01 20:32 UTC

@bmeluch bmeluch marked this pull request as ready for review July 10, 2024 22:16
Copy link

@kheal kheal left a comment

Choose a reason for hiding this comment

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

Reviewed bc this will not affect workflows during the refactoring, but I don't not have sufficient context to say whether or not these terms should be removed, so I'll default to others for that aspect.

@eecavanna
Copy link

I like the PR title! It tells me what will happen on the destination branch if these changes were to be merged into it.

@@ -110,6 +112,7 @@ classes:
- KEGG.ORTHOLOGY prefix is used for KO numbers
todos:
- is OrthologyGroup instantiated in an MongoDB collection? Aren't Pathways searchable in the Data Portal?
deprecated: "not used. 2024-07-10 https://github.com/microbiomedata/nmdc-schema/issues/1881"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We do use the KEGG prefix the both of the aggregation tables which is I think an argument for keeping OrthologyGroup which would also mean keeping FunctionalAnnotationTerm.
i think what should happen is the range of gene_function_id should be OrthologyGroup plus a structured_pattern.syntax constraint @turbomam

Copy link
Author

Choose a reason for hiding this comment

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

@SamuelPurvine this is what we were talking about on Wednesday, yes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, and is why I'd brought it up :)

Copy link
Author

Choose a reason for hiding this comment

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

@aclum @SamuelPurvine @mslarae13

Making sure I understand various conversations - if you will have a use for OrthologyGroup and parent classes, I should close this PR without merging, leave the classes in the schema, and they will be used after berkeley rollout when you make changes to gene_function_id?

@bmeluch bmeluch changed the title Deprecate EnvironmentalMaterialTerm, FunctionalAnnotationTerm + children Deprecate EnvironmentalMaterialTerm, Pathway Aug 1, 2024
@bmeluch
Copy link
Author

bmeluch commented Aug 1, 2024

I removed the deprecation for FunctionalAnnotationTerm and OrthologyGroup since there are plans to use those classes. @aclum and/or @SamuelPurvine could you confirm that this is fixed for what you need?

@SamuelPurvine
Copy link
Collaborator

That'll be a @aclum thing :)

@aclum
Copy link
Collaborator

aclum commented Aug 5, 2024

Approved this PR.

@bmeluch bmeluch merged commit 1ce8a6a into main Aug 7, 2024
3 checks passed
@bmeluch bmeluch deleted the 1881-deprecate-unused-classes branch August 7, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

7 participants