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

type slot assertion on planned NEON process classes and processed sample #1067

Closed
wants to merge 1 commit into from

Conversation

sujaypatil96
Copy link
Collaborator

Fixes #1027 and #1030

@sujaypatil96 sujaypatil96 linked an issue Aug 15, 2023 that may be closed by this pull request
@sujaypatil96
Copy link
Collaborator Author

Agenda item for 8/16 call with @turbomam:

  • decide how we want to constrain the range of type slot on the NEON PlannedProcess and ProcessedSample classes.

@aclum
Copy link
Contributor

aclum commented Nov 1, 2023

@turbomam can we get this reviewed and merged? @sujaypatil96 is there corresponding migration code?

@turbomam
Copy link
Member

turbomam commented Nov 1, 2023

This can be closed, but we need to refactor "type" slots ASAP. I have an issue for this but haven't found it yet.

@aclum
Copy link
Contributor

aclum commented Nov 1, 2023

@turbomam are you saying you'd like to close instead of merge this pull request where @sujaypatil96 has addressed some of the type slot issues?

@turbomam
Copy link
Member

turbomam commented Nov 1, 2023

I was hoping to

  • prioritize applying type, as an inherited type-designating slot (whose range would be identifiers of nmdc-schema classes) to root classes
  • remove all other NMDC slots with "type" in their name. (We would keep any MIxS slots that have "type" in the name)
  • replace the removed slots with xyz_category slots, whose ranges would be enumerated strings

Maybe I'm not the best judge of when to do that. If it's beneficial to start populating the string type slot for the classes in the issue/PR now, then yes, we can merge it.

@aclum
Copy link
Contributor

aclum commented Nov 1, 2023

If we can address your first bullet 'prioritize applying type, as an inherited type-designating slot (whose range would be identifiers of nmdc-schema classes) to root classes' with the december refactor we should do that in favor on merging this in. let's leave this open until we decided @lamccue @mslarae13

@mslarae13
Copy link
Contributor

I'll defer to @lamccue for the prioritization & if there's time to target type for December.

As for getting it done, @turbomam can we get a list of all the slots that have "type" in the name/title that shouldn't so we can evaluate how many slots we need to change?

@lamccue
Copy link
Collaborator

lamccue commented Nov 22, 2023

I don't have a good understanding of the level of effort required, or the impact the change would have within the schema, of the first point, i.e.,:

  1. prioritize applying type, as an inherited type-designating slot (whose range would be identifiers of nmdc-schema classes) to root classes

@mslarae13
Copy link
Contributor

Thanks @lamccue ... @turbomam can we get a list of all the slots that have "type" in the name/title that shouldn't so we can evaluate how many slots we need to change?

@aclum
Copy link
Contributor

aclum commented Dec 5, 2023

Just a note that we can separate concerns here wrt to what gets done during the December refactor, we can add slot 'type' on all classes and set a range to be a class independent of changing the name of slots which currently have the word type in them (ie data_object_type).

@aclum
Copy link
Contributor

aclum commented Dec 21, 2023

Im going to close this PR since @turbomam took care of what is addressed in this PR globally in the berkeley-schema-fy24 fork. I unlinked the nmdc-schema ticket so that would remain open.

@aclum aclum closed this Dec 21, 2023
@turbomam turbomam deleted the assert-type-slot branch January 30, 2024 14:50
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 type on class PlannedProcess
5 participants