Structure validation

[viktorpm]

Description

What is this PR

• Bug fix
• Addition of a new feature
• Other

Why is this PR needed?

We want to test if all the atlases fit the brainglobe framework

What does this PR do?

Adds a new validation function called validate_mesh_structure_pairs to the existing validate_atlases.py script

References

See PR #90

How has this PR been tested?

It was run locally to test if the new function works on existing data

Is this a breaking change?

No

Does this PR require an update to the documentation?

Not for now

Checklist:

• The code has been tested locally
• Tests have been added to cover all new functionality (unit & integration)
• The documentation has been updated to reflect any changes
• The code has been formatted with pre-commit

[codecov]

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (93e95ab) 0.00% compared to head (6183091) 0.00%.

Files	Patch %	Lines
bg_atlasgen/validate_atlases.py	0.00%	16 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #110   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files         24      24           
  Lines       1927    1943   +16     
=====================================
- Misses      1927    1943   +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alessandrofelder]

Good start - I've made a suggestion about how to make this new validation function could be made more consistent with the other validation functions, so it integrates smoothly into the script.

[alessandrofelder]

For consistency with the other validation functions, we should raise an AssertionError with the appropriate error message here, I think?

[alessandrofelder]

why do we need the if part here? Should not all items in the json_file have an id key?
I wonder whether we should check the id via the bg_atlasapi via something like

atlas = BrainGlobeAtlas(atlas_name)
bg_atlas_api_ids = list(atlas.structures.keys())

# a fancy, one-line way to get the ids from the meshes - see https://docs.python.org/3/library/pathlib.html?highlight=stem#pathlib.PurePath.stem
ids_from_mesh_files = [
        int(Path(file).stem) for file in os.listdir(obj_path) if file.endswith(".obj")
    ] 

in_mesh_not_bg = []
for id in ids_from_mesh_files:
    if id not in bg_atlas_api_ids:
        in_mesh_not_bg.append(id)

in_bg_not_mesh = []
for id in bg_atlas_api_ids:
    if id not in ids_from_mesh_files:
        in_bg_not_mesh.append(id)

if len(in_mesh_not_bg) or len(in_bg_not_mesh):
    raise AssertionError(f"Structures with ID {in_bg_not_mesh} are in the atlas, but don't have a corresponding mesh file; Structures with IDs {in_mesh_not_bg} have a mesh file, but are not accessible through the atlas.")

This is designed to throw at most one assertion error, so successful and failed validation results get logged in the same way as other validation functions (see next comment).

[alessandrofelder]

We should at least make an issue to write tests for this function if we don't add them in this PR.

I meant to request changes - sorry!

[alessandrofelder]

Good start - I've made a suggestion about how to make this new validation function could be made more consistent with the other validation functions, so it integrates smoothly into the script.

[alessandrofelder]

Thanks @viktorpm - largely looks good but there are some easily resolvable merge conflicts now that #90 has been merged - could you have a go at resolving them please?