-
Notifications
You must be signed in to change notification settings - Fork 2
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: Adding multi tomogram relationships #302
Conversation
* fix: support filtering entities by related object id's
* Updates to schema and template * Fixing tests * Adding files to alignments metadata * Update the alignment configs method type * Fixing typo * Adding undefined alignment_method_type
* Adds comments to jensen config generation * Adding guardrails * Clean up * fix: method_name
self.local_metadata = { | ||
"object_count": 0, | ||
"files": [], | ||
"alignment_metadata_path": alignment_metadata_path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can only be one alignment associated to an annotation.
@@ -78,7 +78,6 @@ def get_filename_prefix(self, output_dir: str, identifier: int) -> str: | |||
output_dir, | |||
"-".join( | |||
[ | |||
str(identifier), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uermel Should the annotation object state also be included in the file name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please. This was something I asked for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm my understanding, this has already been included in annotation hash used for generating the identifier (ref here). We also want this in the filename?
if hasattr(self, "identifier") and self.identifier: | ||
glob_vars[f"{self.type_key}_id"] = self.identifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handles for the introduction of ids in the file paths.
TiltSeriesImporter: { | ||
RawTiltImporter: {}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The raw tilts are now being added as a child to tiltseries. This means aw tilts files will be imported only if there is a related tiltseries in the config.
@uermel Would this break any expected patterns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me. I don't expect anything to be broken.
This needs to include
This should be |
There was a problem hiding this 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, slight updates to the description may be useful for future reference.
) | ||
return all(volume_dim.get(dim) for dim in "xyz") or self.get_tomogram() is not None | ||
|
||
def get_tomogram(self) -> Optional["TomogramImporter"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should leave a comment as to why this is safe -- basically that we only allow one alignment & voxel spacing per ingestion config, and that we assume all tomograms imported for a single run have the same dimensions.
@@ -24,23 +24,32 @@ class VisualizationConfigImporter(BaseImporter): | |||
finder_factory = DefaultImporterFactory | |||
has_metadata = False | |||
dir_path = ( | |||
"{dataset_name}/{run_name}/Tomograms/VoxelSpacing{voxel_spacing_name}/CanonicalTomogram/" | |||
"neuroglancer_config.json" | |||
"{dataset_name}/{run_name}/Reconstructions/VoxelSpacing{voxel_spacing_name}/NeuroglancerPrecompute/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why aren't we putting neuroglancer precompute data in a subdirectory of the tomogram, sort of like this?
{dataset_id}/{run_name}/Reconstructions/Tomograms/{tomo_id}/NeuroglancerPrecompute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The annotations precompute should be shareable across different tomograms, if they have the same alignments. So, I didn't want to tie it to a tomogram.
There was a problem hiding this 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 once @uermel 's requests are handled.
Relates to chanzuckerberg/cryoet-data-portal#1053
Description
There are general updates to the file paths. More on this can be found here.
Alignments
alignment_id
name inside the Alignment folder"alignments"
. This is to prevent key clashes between any other identifier that is generated at a run level.Annotations
Tomograms/VoxelSpacingX.XXX/Annotations
->Reconstructions/VoxelSpacingX.XXX/Annotations/{annotation_id}
"annotation"
. This is to prevent key clashes between any other identifier that is generated at a voxel spacing level level.annotation_object.state
and relatedalignment_metadata_path
Key Photos
Tomograms/VoxelSpacingX.XXX/KeyPhotos
->Reconstructions/VoxelSpacingX.XXX/Images/
Raw Tilts
TiltSeries
->TiltSeries/{tiltseries_id}/
Tilt Series
TiltSeries
->TiltSeries/{tiltseries_id}/
Tomograms
Tomograms/VoxelSpacingX.XXX/CanonicalTomogram
->Reconstructions/VoxelSpacingX.XXX/Tomograms/{tomogram_id}
alignment_metadata_path
andneuroglancer_config_path
.neuroglancer_config_path
is only set to a non-null value ifis_visualization_default=true
Visualization Config
Tomograms/VoxelSpacingX.XXX/CanonicalTomogram
->Reconstructions/VoxelSpacingX.XXX/NeuroglancerPrecompute/
is_visualization_default = true
Annotation Precompute
Tomograms/VoxelSpacingX.XXX/NeuroglancerPrecompute
->Reconstructions/VoxelSpacingX.XXX/NeuroglancerPrecompute/