Improve core test coverage

[stellaprins]

Description

What is this PR

• Bug fix
• Addition of a new feature
• Other (add tests)

Why is this PR needed?
To make code in brainglobe-atlasapi more robust by increasing test coverage.

What does this PR do?
Adds test for uncovered method get_structure_mask in core.

References

#514

How has this PR been tested?

Is this a breaking change?

No.

Does this PR require an update to the documentation?

No.

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

[stellaprins]

This PR does not cover all lines of core. This could be done if there is time left after adding tests for the atlas generation functions (#503).

[adamltyson]

Could you explain this a bit more? Is this referring to the hierarchy, i.e. the smaller regions contained within the larger region?

[stellaprins]

It does not refer to the hierarchy, but yes, you're right, it is a bit confusing and looking at this again, maybe I should have reassigned the label numbers in the annotation to match the structure ids instead of reassigning the structure ids? I've tried to make the docstring a bit clearer, it now reads. Let me know what you think!

Test the get_structure_mask method.

>>> atlas.structures
root (997)
  └── grey (8)
        └── CH (567)

The 'structures' "grey" and "CH" are present in the example atlas. Their
respective ids are 8 and 567. These labels are not present in the
annotation of the example atlas however. Because the labels 7 and 566
are present, we reassign the parent and substructure ids to match the
annotation for testing purposes.

Because the "CH" structure is a sub-structure of "grey" it should adopt
the parent structure id (7) in the mask where its label (566) is present
when get_structure_mask is applied.

After applying get_structure_mask only the parent structure id (7) should
remain in the mask for the regions corresponding to "CH" and "grey".

All labels belonging to structures that are outside of the parent structure
should be set to 0.

[adamltyson]

Would it be possible to add a few more comments about these tests? They look technically fine, but I don't immediately understand what exactly they are doing and why. While I could be less lazy and look into a bit more, I assume that if they confuse me, they will also confuse a new contributor.

[stellaprins]

Would it be possible to add a few more comments about these tests? They look technically fine, but I don't immediately understand what exactly they are doing and why. While I could be less lazy and look into a bit more, I assume that if they confuse me, they will also confuse a new contributor.

It took me a while to figure this out so it would be good to sanity check whether what I have done makes sense. I've added clarification to the test docstring and hope that the example below further clarifies why I reassign the structure ids.

from brainglobe_atlasapi.bg_atlas import BrainGlobeAtlas
atlas = BrainGlobeAtlas("example_mouse_100um")
print(atlas.structures)
non_zero_locations = np.nonzero(atlas.annotation)
non_zero_values = atlas.annotation[non_zero_locations]
labels_in_annotation = np.unique(non_zero_values)
print(f"grey (8) present in annotation: {8 in labels_in_annotation}")
print(f"CH (567) present in annotation: {567 in labels_in_annotation}\n")
print(f"7 present in annotation: {7 in labels_in_annotation}")
print(f"566 present in annotation: {566 in labels_in_annotation}")

out:

root (997)
  └── grey (8)
        └── CH (567)

grey (8) present in annotation: False
CH (567) present in annotation: False

7 present in annotation: True
566 present in annotation: True

[adamltyson]

Yep, makes sense, thanks!