Conversation
|
Two suggestions:
|
gouwens
left a comment
gouwens
left a comment
There was a problem hiding this comment.
Overall looks good - just a question about mating status being required or optional.
| sex: Sex = Field(..., title="Sex") | ||
| date_of_birth: Optional[Annotated[date_type, TimeValidation.BEFORE]] = Field(default=None, title="Date of birth") | ||
| year_of_birth: int = Field(..., title="Year of birth") | ||
| mating_status: MatingStatus = Field(..., title="Mating status") |
There was a problem hiding this comment.
I'm not sure, but maybe mating status should also be optional for NHP subject?
There was a problem hiding this comment.
we do have an "unknown" option, which I think is a bit better than having it optional because then people are explicit about not knowing vs having to guess did they just not bother to provide it or is it unknown?
But we can make it optional too...
There was a problem hiding this comment.
Got it - having "unknown" works for me, then
dbirman
left a comment
dbirman
left a comment
There was a problem hiding this comment.
Need to add a validator for HumanSubject.species to avoid crashing old code (unless we're sure none of these assets exist)
| class HumanSubject(DataModel): | ||
| """Description of a human subject""" | ||
|
|
||
| species: Species.ONE_OF = Field(..., title="Species") |
There was a problem hiding this comment.
I feel like there's a sensible default we could put here. More seriously this is a breaking change, we should add a field_validator in mode="before" that pre-fills this if it's not present to avoid crashing old assets (if any have HumanSubject, I'm not sure they do though)
There was a problem hiding this comment.
It gave me all sorts of errors when I tried to make it a default and I didn't have the patience to figure out why. But yes, I agree.
I think there is one very old exaspim asset with human data, that may or may not have any of the rest of its metadata...I'll look into it. But yes, let's not break everything.
3 participants
closes #1701