-
Notifications
You must be signed in to change notification settings - Fork 37
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
Integrating atlas validation in wrapup.py #445
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Example output for
Catches known error. |
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.
Lovely.
Two small comments for simplification :)
…if all validations passed
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.
Thanks @viktorpm
Sorry - I wasn't clear enough in my description of the changes I suggested, which (if I am right??) would significantly simplify the code even further. (Let's chat if you have questions!).
@@ -174,18 +196,28 @@ def catch_missing_mesh_files(atlas: BrainGlobeAtlas): | |||
) | |||
|
|||
|
|||
def catch_missing_structures(atlas: BrainGlobeAtlas): | |||
def catch_missing_structures( | |||
atlas: BrainGlobeAtlas, custom_atlas_path: Path = None |
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.
atlas: BrainGlobeAtlas, custom_atlas_path: Path = None | |
atlas: BrainGlobeAtlas |
if custom_atlas_path is None: | ||
atlas_path = ( | ||
Path(get_brainglobe_dir()) / f"{atlas.atlas_name}_v" | ||
f"{get_local_atlas_version(atlas.atlas_name)}" | ||
) | ||
else: | ||
atlas_path = custom_atlas_path | ||
|
||
obj_path = Path(atlas_path / "meshes") |
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.
if custom_atlas_path is None: | |
atlas_path = ( | |
Path(get_brainglobe_dir()) / f"{atlas.atlas_name}_v" | |
f"{get_local_atlas_version(atlas.atlas_name)}" | |
) | |
else: | |
atlas_path = custom_atlas_path | |
obj_path = Path(atlas_path / "meshes") | |
obj_path = Path(atlas.root_dir/ "meshes") |
This is what I had in mind (and for other functions for which this PRs adds a custom_atlas_path
parameter.
(validate_atlas_files, atlas_to_validate, atlas_to_validate.root_dir), | ||
( | ||
catch_missing_structures, | ||
atlas_to_validate, | ||
atlas_to_validate.root_dir, | ||
), | ||
( | ||
catch_missing_mesh_files, | ||
atlas_to_validate, | ||
atlas_to_validate.root_dir, | ||
), | ||
(validate_mesh_matches_image_extents, atlas_to_validate), | ||
(open_for_visual_check, atlas_to_validate), | ||
(validate_checksum, atlas_to_validate), | ||
(validate_image_dimensions, atlas_to_validate), | ||
(validate_additional_references, atlas_to_validate), |
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.
If we do the changes I suggested, this can just be a list of functions (without atlas_to_validate
or atlas_to_validate.root_dir
!) and we can just call func(atlas_to_validate)
below
Description
This PR integrates our atlas validation functions in the wrapup section of atlas packaging.
What is this PR
Why is this PR needed?
Currently, we run the validation functions on finalised atlases. We want to integrate this step with atlas generation to see if an atlas is valid or not immediately.
References
?
How has this PR been tested?
Locally on
Is this a breaking change?
No
Does this PR require an update to the documentation?
Possibly
Checklist: