feat: Metadata.from_metadata#1852
Conversation
tmchartrand
left a comment
tmchartrand
left a comment
There was a problem hiding this comment.
I'll look through in a bit more detail later, but one high-level thought:
I think we should clarify that we still want to inherit subject metadata if there are additional input assets without subject metadata (ie aggregated reference data, templates or the like) in addition to the single-subject assets (and same for sessions).
For example, if someone uses the CCF to do registration that doesn't mean their derived asset is "multi-subject"? I agree we can add that clarification. |
|
Cool I'm pretty sure the code handles this correctly as-is, but we should update docstrings and docs pages and maybe have explicit tests. |
tmchartrand
left a comment
tmchartrand
left a comment
There was a problem hiding this comment.
Looks mostly good but I think we at least need to resolve some open questions about the inheritance of names and other fields of data_description.
| if value is None: | ||
| continue | ||
|
|
||
| field_class = [ |
There was a problem hiding this comment.
the values should already be instances of the model classes, why retrieve the class at all? can't you just call the classmethods model_validate or model_construct on the instances?
There was a problem hiding this comment.
It's defensive against a dictionary being passed as a core file. I was being careful because, for example, we know all procedures metadata is invalid. But now that you mention it it's probably overly defensive? I think even if you call model_construct on an invalid procedures dict it will still construct a Procedures object, it will just have dictionaries somewhere inside wherever the validation fails to produce the right objects. I'll double check that this is true before changing it though.
| derived_dd = DataDescription.from_data_description( | ||
| first_dd, | ||
| process_name=process_name, | ||
| source_data=source_names if len(source_names) > 1 else None, | ||
| **data_description_kwargs, |
There was a problem hiding this comment.
I don't think building the datadescription from the first input is correct unless there's only one acquisition (thus all have the same original input name). Even then it's a bit weird that fields like data_summary that may not be shared get inherited effectively randomly (based on order). For multi-acquisition we at least need to drop the datetime part of the name I think (or use the latest one, not sure we have a clear standard), and for multi-subject build a new name following that pattern.
Another issue is making it clear to the user that fields like dd.project should be overridden if the processing is for a different project than the original data (possible here via kwargs but nonobvious).
This is maybe a longer conversation, but I actually think we should be deprecating and/or making private the data_description.from_xxx methods, and replacing with something in the inheritance module that follows the logic outlined above.
There was a problem hiding this comment.
I think deprecating those is fine and we can drop them in biodata-schema, let me take a pass at making specific helper functions for each situation so that they're really clear and then lets see how it looks.
| def from_metadata( | ||
| cls, | ||
| metadata: "Union[Metadata, List[Metadata]]", | ||
| process_name: str, |
There was a problem hiding this comment.
why does it need the process_name, isn't that already in the Processing object passed to it?
There was a problem hiding this comment.
and why would the process_name be required when the Processing input is optional?
There was a problem hiding this comment.
okay, I get what the process_name is now - but I don't think I understand how this works when there is no processing. What should the process_name be then?
There was a problem hiding this comment.
good point, I would say that new_processing should be required, and new_process be renamed process_short_name (optional, by default derived via some heuristics from the Processing.name)
There was a problem hiding this comment.
If you get a whole bunch of processes how would you know which one to use as the new label for the derived asset? I think it's better to let the user control this.
There was a problem hiding this comment.
at some point we had guidance for people to use the last processing step as the label, BUT, I'm okay letting users control this. I think there might need to be some information in the docs/docstring about how this is being used so that they provide it well.
There was a problem hiding this comment.
I don't think we want last processing step in general, when you do e.g. spikesorting you would get something really weird. The current final step (in a random asset I checked) is Ephys visualization - experiment1_Record Node 101#Neuropix-PXI-116.46105_recording1, whereas ideally it would say spikesorted. I think that's a general pattern across pipelines.
Maybe we add this (or a similar) description: "Abbreviated name for the process or pipeline that was run. Defaults to 'processed'"
There was a problem hiding this comment.
"Shortened name for the process or pipeline that was run - used in creating name of derived asset. Defaults to 'processed'"
saskiad
left a comment
saskiad
left a comment
There was a problem hiding this comment.
I'm confused on the process_name when there is no processing.
3 participants
This PR adds a new function
Metadata.from_metadatathat simplifies the process of generating derived asset metadata. It implements four rules (see the docs page) that control how metadata is inherited.