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

feat: moves aind_data_schema.models here #1

Merged
merged 5 commits into from
May 7, 2024
Merged

Conversation

jtyoung84
Copy link
Contributor

  • Ports aind_data_schema.models to an isolated repo
  • Changed field_validators to model_validators in devices class for validators that require referencing other fields
  • Adds unit tests for 100% coverage
  • Fixed typo in AuditoryStimulation stimulus_name (was originally sitmulus_name)

@jtyoung84 jtyoung84 requested review from dyf and saskiad April 27, 2024 21:48
@saskiad
Copy link
Collaborator

saskiad commented Apr 29, 2024

I'm not sure I think it makes sense to move all of this out of the aind-data-schema repo. Eg. things like coordinates and base seem like they should be in the schema itself. The ones that definitely need to be in the model repo are:
modalities
organizations
platforms
process_names
registry
species
harp_types

I think devices, coordinates, pid_names, and reagent should be part of the schema. Probably stimulus too - this one I'm hoping we can wean ourselves off of having these classes, but I think it belongs in the schema for now.

I'm ambivalent about units

@dyf
Copy link
Member

dyf commented Apr 29, 2024

I'm not sure I think it makes sense to move all of this out of the aind-data-schema repo. Eg. things like coordinates and base seem like they should be in the schema itself. The ones that definitely need to be in the model repo are: modalities organizations platforms process_names registry species harp_types

I think devices, coordinates, pid_names, and reagent should be part of the schema. Probably stimulus too - this one I'm hoping we can wean ourselves off of having these classes, but I think it belongs in the schema for now.

I'm ambivalent about units

Agree with this list. I think units can live here.

Copy link
Collaborator

@saskiad saskiad left a comment

Choose a reason for hiding this comment

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

Looks good to me

@jtyoung84 jtyoung84 added this pull request to the merge queue May 7, 2024
Merged via the queue into main with commit baf3e16 May 7, 2024
4 checks passed
@jtyoung84 jtyoung84 deleted the feat-init-commit branch May 7, 2024 00:07
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.

3 participants