-
Notifications
You must be signed in to change notification settings - Fork 24
draft validation functions #90
Changes from 20 commits
dec80c1
566fc44
8bb2150
8aa73e7
9423015
1fcafa4
319e721
22d6c96
9402b66
598a0e1
96c8584
ffef504
92e94e6
29343dc
5995c43
cb9cc02
85419d8
dc4aebc
a59da92
f06c82f
dace6b3
9faf48f
777d309
b8377c5
8736701
b517507
26b9dcf
f7fa093
af84ec3
bd1f185
d0f81ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,124 @@ | ||
"""Script to validate atlases""" | ||
|
||
|
||
from pathlib import Path | ||
|
||
import numpy as np | ||
from bg_atlasapi import BrainGlobeAtlas | ||
from bg_atlasapi.config import get_brainglobe_dir | ||
from bg_atlasapi.list_atlases import ( | ||
get_all_atlases_lastversions, | ||
get_atlases_lastversions, | ||
) | ||
from bg_atlasapi.update_atlases import update_atlas | ||
|
||
|
||
def validate_atlas_files(atlas_path: Path): | ||
"""Checks if basic files exist in the atlas folder""" | ||
|
||
assert atlas_path.exists(), f"Atlas path {atlas_path} not found" | ||
expected_files = [ | ||
"annotation.tiff", | ||
"reference.tiff", | ||
"metadata.json", | ||
"structures.json", | ||
"meshes", | ||
] | ||
for expected_file_name in expected_files: | ||
expected_path = Path(atlas_path / expected_file_name) | ||
assert ( | ||
expected_path.exists() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, here one could be stricter and test |
||
), f"Expected file not found at {expected_path}" | ||
return True | ||
|
||
|
||
def _assert_close(mesh_coord, annotation_coord, pixel_size): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. type annotations are missing here. I presume Would also be helpful to have a full docstring with expected parameters and returns here (since there are several arguments). |
||
"""Helper function to check if the mesh and the annotation coordinate are closer to each other than 10 times the pixel size""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docstring exceeds usual line length limits, use multi-line strings? """Helper function to check if the mesh and the annotation coordinate are
closer to each other than 10 times the pixel size""" |
||
assert ( | ||
abs(mesh_coord - annotation_coord) <= 10 * pixel_size | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here 10 is an arbitrary "magic number". I guess it works well in this case, but I could imagine us wanting to change that for some other validation check. Would it make sense to expose that as an argument? |
||
), f"Mesh coordinate {mesh_coord} and annotation coordinate {annotation_coord} differ by more than 10 times pixel size {pixel_size}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make this into a multi-line string to avoid violating line length limits |
||
return True | ||
|
||
|
||
def validate_mesh_matches_image_extents(atlas: BrainGlobeAtlas): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is longish, with some repetitions. It could use some further refactoring imo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For example, you could extract some bits as further helper functions (the way it's done for |
||
"""Checks if the mesh and the image extents are similar""" | ||
|
||
root_mesh = atlas.mesh_from_structure("root") | ||
annotation_image = atlas.annotation | ||
resolution = atlas.resolution | ||
|
||
z_range, y_range, x_range = np.nonzero(annotation_image) | ||
z_min, z_max = np.min(z_range), np.max(z_range) | ||
y_min, y_max = np.min(y_range), np.max(y_range) | ||
x_min, x_max = np.min(x_range), np.max(x_range) | ||
|
||
mesh_points = root_mesh.points | ||
z_coords, y_coords, x_coords = ( | ||
mesh_points[:, 0], | ||
mesh_points[:, 1], | ||
mesh_points[:, 2], | ||
) | ||
z_min_mesh, z_max_mesh = np.min(z_coords), np.max(z_coords) | ||
y_min_mesh, y_max_mesh = np.min(y_coords), np.max(y_coords) | ||
x_min_mesh, x_max_mesh = np.min(x_coords), np.max(x_coords) | ||
|
||
z_min_scaled, z_max_scaled = z_min * resolution[0], z_max * resolution[0] | ||
y_min_scaled, y_max_scaled = y_min * resolution[1], y_max * resolution[1] | ||
x_min_scaled, x_max_scaled = x_min * resolution[2], x_max * resolution[2] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this block operates on the annotation image, I would move it further above, just after If you decide to follow my advice on refactoring this function, the above points may be rendered mute anyway. |
||
|
||
_assert_close(z_min_mesh, z_min_scaled, resolution[0]) | ||
_assert_close(z_max_mesh, z_max_scaled, resolution[0]) | ||
_assert_close(y_min_mesh, y_min_scaled, resolution[1]) | ||
_assert_close(y_max_mesh, y_max_scaled, resolution[1]) | ||
_assert_close(x_min_mesh, x_min_scaled, resolution[2]) | ||
_assert_close(x_max_mesh, x_max_scaled, resolution[2]) | ||
|
||
return True | ||
|
||
|
||
def open_for_visual_check(atlas): | ||
pass | ||
|
||
|
||
def validate_checksum(atlas): | ||
pass | ||
|
||
|
||
def check_additional_references(): | ||
# check additional references are different, but have same dimensions | ||
pass | ||
|
||
|
||
def validate_atlas(atlas_name, version): | ||
"""Validates the latest version of a given atlas""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the atlas get updated to latest during validation, why do you need to pass the version as an argument? Even if you pass an older version, it will be overriden by the update, and the function will actually validate the newer version. Or am I wrong? |
||
|
||
print(atlas_name, version) | ||
atlas = BrainGlobeAtlas(atlas_name) | ||
updated = get_atlases_lastversions()[atlas_name]["updated"] | ||
if not updated: | ||
update_atlas(atlas_name) | ||
atlas_path = Path(get_brainglobe_dir()) / f"{atlas_name}_v{version}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't the version change after the update? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you would have to get the version after the update |
||
assert validate_atlas_files( | ||
atlas_path | ||
), f"Atlas file {atlas_path} validation failed" | ||
assert validate_mesh_matches_image_extents( | ||
atlas | ||
), "Atlas object validation failed" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Atlas object validation" is too abstract for this message. You didn't check the entire object, just the extents of the annotation image and the mesh, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless this message anticipates the addition of more checks in this group |
||
|
||
|
||
if __name__ == "__main__": | ||
valid_atlases = [] | ||
invalid_atlases = [] | ||
for atlas_name, version in get_all_atlases_lastversions().items(): | ||
try: | ||
validate_atlas(atlas_name, version) | ||
valid_atlases.append(atlas_name) | ||
except AssertionError as e: | ||
invalid_atlases.append((atlas_name, e)) | ||
continue | ||
|
||
print("Summary") | ||
print("### Valid atlases ###") | ||
print(valid_atlases) | ||
print("### Invalid atlases ###") | ||
print(invalid_atlases) | ||
Comment on lines
+164
to
+168
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be useful to also save the output in a .txt (or .md) file in addition to printing it? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
from pathlib import Path | ||
|
||
import numpy as np | ||
import pytest | ||
from bg_atlasapi import BrainGlobeAtlas | ||
from bg_atlasapi.config import get_brainglobe_dir | ||
|
||
from bg_atlasgen.validate_atlases import ( | ||
_assert_close, | ||
validate_atlas_files, | ||
validate_mesh_matches_image_extents, | ||
) | ||
|
||
|
||
def test_validate_mesh_matches_image_extents(): | ||
atlas = BrainGlobeAtlas("allen_mouse_100um") | ||
assert validate_mesh_matches_image_extents(atlas) | ||
|
||
|
||
def test_validate_mesh_matches_image_extents_negative(mocker): | ||
atlas = BrainGlobeAtlas("allen_mouse_100um") | ||
flipped_annotation_image = np.transpose(atlas.annotation) | ||
mocker.patch( | ||
"bg_atlasapi.BrainGlobeAtlas.annotation", | ||
new_callable=mocker.PropertyMock, | ||
return_value=flipped_annotation_image, | ||
) | ||
with pytest.raises( | ||
AssertionError, match="differ by more than 10 times pixel size" | ||
): | ||
validate_mesh_matches_image_extents(atlas) | ||
|
||
|
||
def test_valid_atlas_files(): | ||
_ = BrainGlobeAtlas("allen_mouse_100um") | ||
atlas_path = Path(get_brainglobe_dir()) / "allen_mouse_100um_v1.2" | ||
assert validate_atlas_files(atlas_path) | ||
|
||
|
||
def test_invalid_atlas_path(): | ||
atlas_path = Path.home() | ||
with pytest.raises(AssertionError, match="Expected file not found"): | ||
validate_atlas_files(atlas_path) | ||
|
||
|
||
def test_assert_close(): | ||
assert _assert_close(99.5, 8, 10) | ||
|
||
|
||
def test_assert_close_negative(): | ||
with pytest.raises( | ||
AssertionError, match="differ by more than 10 times pixel size" | ||
): | ||
_assert_close(99.5, 30, 2) |
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
exists()
method will return True whether the path in question is a directory or a file.Since we explicitly expect
atlas_path
to be a directory, would it make sense to be stricter and check foratlas_path.is_dir()
?