Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

draft validation functions #90

Merged
merged 31 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
dec80c1
draft validation functions
viktorpm Oct 12, 2023
566fc44
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 12, 2023
8bb2150
run on all atlases, don't crash on assertion error
viktorpm Oct 12, 2023
8aa73e7
fix merging
viktorpm Oct 12, 2023
9423015
fixing atlas path
viktorpm Nov 2, 2023
1fcafa4
Merge pull request #1 from viktorpm/fix-path
alessandrofelder Nov 2, 2023
319e721
Clearer output printing
alessandrofelder Nov 2, 2023
22d6c96
Merge pull request #2 from viktorpm/minor-tweaks-for-validation
viktorpm Nov 2, 2023
9402b66
tidy up validation script, remove weird test_git
alessandrofelder Nov 3, 2023
598a0e1
add dev install, make test structure, initial tests
alessandrofelder Nov 3, 2023
96c8584
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 3, 2023
ffef504
add tests and return for _assert_close()
viktorpm Nov 3, 2023
92e94e6
add test for validate mesh matches annotation
viktorpm Nov 3, 2023
29343dc
fix linting
viktorpm Nov 3, 2023
5995c43
update version for actions
alessandrofelder Nov 7, 2023
cb9cc02
drop py3.8 in tox, run pytest in tox
alessandrofelder Nov 7, 2023
85419d8
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 7, 2023
dc4aebc
fix copy-paste error in pytest command
alessandrofelder Nov 7, 2023
a59da92
drop py3.8 from gh action workflow file too
alessandrofelder Nov 7, 2023
f06c82f
Adding docstrings to validation script
viktorpm Nov 8, 2023
dace6b3
Making path tests stricter, breaking up long strings, adding diff_tol…
viktorpm Nov 24, 2023
9faf48f
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 29, 2023
777d309
restructuring validate_mesh_matches_image_extents function, adding co…
viktorpm Nov 29, 2023
b8377c5
Merge branch 'validation' of github.com:viktorpm/bg-atlasgen into val…
viktorpm Nov 29, 2023
8736701
testing expected files and meshes directory separately
viktorpm Jan 3, 2024
b517507
looping through validation functions and parameters to catch individu…
viktorpm Jan 8, 2024
26b9dcf
removing hard coded path, generalising to all atlases
viktorpm Jan 9, 2024
f7fa093
adding successful_validations list
viktorpm Jan 10, 2024
af84ec3
tidying up duplications
viktorpm Jan 16, 2024
bd1f185
fix recursive bug
alessandrofelder Jan 16, 2024
d0f81ab
addressing Niko's final comments, cleaning code
viktorpm Jan 22, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions .github/workflows/test_and_deploy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ jobs:
lint:
runs-on: ubuntu-latest
steps:
- uses: neuroinformatics-unit/actions/lint@v1
- uses: neuroinformatics-unit/actions/lint@v2

test:
needs: lint
Expand All @@ -25,10 +25,8 @@ jobs:
python-version: "3.10"
- os: windows-latest
python-version: "3.9"
- os: ubuntu-latest
python-version: "3.8"

steps:
- uses: neuroinformatics-unit/actions/test@v1
- uses: neuroinformatics-unit/actions/test@v2
with:
python-version: ${{ matrix.python-version }}
21 changes: 0 additions & 21 deletions bg_atlasgen/test_git.py

This file was deleted.

135 changes: 135 additions & 0 deletions bg_atlasgen/validate_atlases.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
"""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.is_dir(), 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.is_file()
), f"Expected file not found at {expected_path}"
Comment on lines +28 to +30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @viktorpm, the test failure is caused by this assertion. The problem is that you are checking if all elements in expected_files are indeed existing files (as I suggested), but "meshes" is a folder not a file. I would check the meshes separately with is_dir(), for example:

assert atlas_path.is_dir(), f"Atlas path {atlas_path} not found"
expected_files = [
    "annotation.tiff",
    "reference.tiff",
    "metadata.json",
    "structures.json",
]
for expected_file_name in expected_files:
    expected_path = Path(atlas_path / expected_file_name)
    assert (
        expected_path.is_file()
    ), f"Expected file not found at {expected_path}"
meshes_path = atlas_path / "meshes"
assert meshes_path.is_dir(), f"Meshes path {meshes_path} not found"
return True

It's important that you check the meshes folder after you check individual files, otherwise the test_invalid_atlas_path() will fail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! It's fixed now

return True


def _assert_close(mesh_coord, annotation_coord, pixel_size, diff_tolerance=10):
"""
Helper function to check if the mesh and the annotation coordinate
are closer to each other than an arbitrary tolerance value times the pixel size.
The default tolerance value is 10.
"""
assert abs(mesh_coord - annotation_coord) <= diff_tolerance * pixel_size, (
f"Mesh coordinate {mesh_coord} and annotation coordinate {annotation_coord}",
f"differ by more than {diff_tolerance} times pixel size {pixel_size}",
)
return True


def validate_mesh_matches_image_extents(atlas: BrainGlobeAtlas):
Copy link
Member

@niksirbi niksirbi Nov 14, 2023

Choose a reason for hiding this comment

The 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.
You don't have to as it's perfectly readable and works fine.

Copy link
Member

Choose a reason for hiding this comment

The 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 _assert_close), or you could introduce some for loops (as long as they don't interfere with readability too much)

"""Checks if the mesh and the image extents are similar"""

root_mesh = atlas.mesh_from_structure("root")
annotation_image = atlas.annotation
resolution = atlas.resolution

# minimum and maximum values of the annotation image (z, y, x)
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)

# minimum and maximum values of the annotation image scaled by the atlas resolution
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]

# z, y and x coordinates of the root mesh (extent of the whole object)
mesh_points = root_mesh.points
z_coords, y_coords, x_coords = (
mesh_points[:, 0],
mesh_points[:, 1],
mesh_points[:, 2],
)

# minimum and maximum coordinates of the root mesh
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)

# checking if root mesh and image are on the same scale
_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"""
Copy link
Member

Choose a reason for hiding this comment

The 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}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't the version change after the update?

Copy link
Member

Choose a reason for hiding this comment

The 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"
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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?

19 changes: 13 additions & 6 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,19 @@ allenmouse = [
"allensdk",
]

dev = [
"pytest",
"pytest-cov",
"pytest-mock",
"coverage",
"tox",
"black",
"mypy",
"pre-commit",
"ruff",
"setuptools_scm",
]

[build-system]
requires = [
"setuptools>=45",
Expand All @@ -59,12 +72,6 @@ include-package-data = true
[tool.setuptools.packages.find]
include = ["bg_atlasgen*"]

[tool.pytest.ini_options]
addopts = "--cov=bg_atlasgen"
filterwarnings = [
"error",
]


[tool.black]
target-version = ['py38', 'py39', 'py310', 'py311']
Expand Down
Empty file added tests/__init__.py
Empty file.
54 changes: 54 additions & 0 deletions tests/test_unit/test_validation.py
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)
5 changes: 2 additions & 3 deletions tox.ini
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
[tox]
envlist = py{38,39,310,311}
envlist = py{39,310,311}

[gh-actions]
python =
3.8: py38
3.9: py39
3.10: py310
3.11: py311
Expand All @@ -12,4 +11,4 @@ python =
extras =
dev
commands =
python -c "import bg_atlasgen"
pytest -v --color=yes --cov=bg_atlasgen --cov-report=xml
Loading