Update thalamus

[asoplata]

(This includes the Ultraliser change I made a different PR for)

This makes significant changes to the creation of placement-hints for the thalamus, in addition to adding more documentation on what is going on to create them. The new instructions are included in the docstring for the ThalamusAtlas class.

The only thing left to decide is where to (ideally permanently) store the handcut reticular meshes that are manually made in this process. The handcut meshes will need to be updated whenever there's a change to the thalamus annotation (I expect very few instances of this by the end of 2024, maybe only 1 or 2 times). However, if the thalamus annotation has not been changed, then the placement-hints can be freely regenerated using the handcut meshes whenever desired, e.g. whenever placement-hints for the entire atlas pipeline are generated.

[asoplata]

Regarding the remaining issue of where to store the handcut meshes needed for the thalamus' placement-hints building, I chatted with @crisely09 and she agreed that the following is allowable:

1. Register the files as entities in an internal BBP-project on the Nexus
2. Put a link to the internal BBP resource on BBP's Nexus into the public code here on github (which would go in app/metadata/thalamus_metadata.json)

@mgeplf Would you be okay with this solution? This way our internal Atlas building process would still be able to access the files, but without adding raw data to the code.

[mgeplf]

@mgeplf Would you be okay with this solution? This way our internal Atlas building process would still be able to access the files, but without adding raw data to the code.

Fine w/ me. How hard would it be to create some test data, so we can at least have an inkling it's working correctly.

[asoplata]

I'm dumb and didn't realize there is a more obvious solution: simply passing the folder with meshes as a CLI argument, and let the rest of the Atlas pipeline/whatever manage the data just like it does with all the other data (annotation NRRD, etc.). I'll probably make this change today, then leave as-is.

Also, Mike, I would say don't worry about this PR for now -- if you look at internal ticket DKE-1224, you'll see that we're going to start having the scientists build Docker images for their region-specific Atlas rules. It's not as clean as properly updating parts of the pipeline like this repo, not to mention avoiding the tests, but it will finally start getting the other circuits integrated into the Atlas build.

[codecov-commenter]

Codecov Report

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

Comparison is base (11e099b) 79.91% compared to head (d183a67) 75.15%.

Files	Patch %	Lines
atlas_placement_hints/layered_atlas.py	15.21%	39 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #11 +/- ## ========================================== - Coverage 79.91% 75.15% -4.76% ========================================== Files 9 9 Lines 453 487 +34 ========================================== + Hits 362 366 +4 - Misses 91 121 +30 ``` | [Flag](https://app.codecov.io/gh/BlueBrain/atlas-placement-hints/pull/11/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain) | Coverage Δ | | |---|---|---| | [pytest](https://app.codecov.io/gh/BlueBrain/atlas-placement-hints/pull/11/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain) | `75.15% <15.21%> (-4.76%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[asoplata]

I've made all the changes I needed to make for the placement-hints to be successfully built, and verified/tested the output to be good, including using the appropriately-built Atlas pipeline inputs. I believe that all that remains is for it to pass the tests, which I don't know why it's not passing.

[arnaudon]

run tox -e format and push the changes made (also run tox -e lint to check it passes the lint CI, then check https://github.com/BlueBrain/atlas-placement-hints/actions/runs/8169361051/job/22333282867?pr=11 where it shows you a diff in a test

[asoplata]

There is a tiny change I forgot to make (due to all the changes required for the Pipeline), so I will make that and then do the tox stuff you mentioned

[arnaudon]

I forgot to mention, to repro the error in the test, just do tox -e py3

[asoplata]

Is there a particular configuration of BB5 modules I can use to build the test environment that tox needs? I've run into the issue I'm having before: despite loading the py-cgal-pybind module, any tox command I try to run in my install on BB5 tries to build a wheel for cgal-pybind. However, this always fails.

In order to build the tox environment on BB5, do I need to load separate modules that contain https://github.com/BlueBrain/cgal-pybind?tab=readme-ov-file#requirements ? I would think those libraries are present in the module for py-cgal-pybind, but loading that module doesn't solve the issue.

Alternatively, should I not be trying to run these tests on BB5? Do I need to instead run this on my local computer (and therefore need to be able to install cgal-pybind myself)?

[arnaudon]

ah yes, this is a pain, what I did is to comment the cgal line in setup.py, so tox does not try to install it. It is not really needed for the test I think

[asoplata]

I've made the changes needed for passing tox -e format, but tox -e lint appears to fail at the pylint step, even though its output seems to only produce suggestions that look like warnings instead of errors to me. Do I need to make all the listed fixes, or is there a different reason it's failing? Here's the output from tox -e lint:

lint: commands[4]> pylint atlas_placement_hints
************* Module atlas_placement_hints.layered_atlas
atlas_placement_hints/layered_atlas.py:39:0: C0413: Import "import trimesh" should be placed at the top of the module (wrong-import-position)
atlas_placement_hints/layered_atlas.py:40:0: C0413: Import "from voxcell import VoxelData" should be placed at the top of the module (wrong-import-position)
atlas_placement_hints/layered_atlas.py:221:41: W0613: Unused argument 'hemisphere' (unused-argument)
atlas_placement_hints/layered_atlas.py:221:58: W0613: Unused argument 'thalamus_meshes_dir' (unused-argument)
atlas_placement_hints/layered_atlas.py:502:20: C0209: Formatting a regular string which could be an f-string (consider-using-f-string)
atlas_placement_hints/layered_atlas.py:507:8: R1722: Consider using 'sys.exit' instead (consider-using-sys-exit)
atlas_placement_hints/layered_atlas.py:563:12: R1722: Consider using 'sys.exit' instead (consider-using-sys-exit)
atlas_placement_hints/layered_atlas.py:580:16: C0209: Formatting a regular string which could be an f-string (consider-using-f-string)
atlas_placement_hints/layered_atlas.py:587:16: C0209: Formatting a regular string which could be an f-string (consider-using-f-string)
atlas_placement_hints/layered_atlas.py:598:16: C0209: Formatting a regular string which could be an f-string (consider-using-f-string)
atlas_placement_hints/layered_atlas.py:510:14: W0613: Unused argument 'layered_volume' (unused-argument)
atlas_placement_hints/layered_atlas.py:39:0: C0411: third party import "trimesh" should be placed before first party imports "atlas_placement_hints.distances.create_watertight_mesh.create_watertight_trimesh", "atlas_placement_hints.distances.distances_to_meshes.distances_from_voxels_to_meshes_wrt_dir", "atlas_placement_hints.utils.centroid_outfacing_mesh"  (wrong-import-order)
atlas_placement_hints/layered_atlas.py:40:0: C0411: third party import "voxcell.VoxelData" should be placed before first party imports "atlas_placement_hints.distances.create_watertight_mesh.create_watertight_trimesh", "atlas_placement_hints.distances.distances_to_meshes.distances_from_voxels_to_meshes_wrt_dir", "atlas_placement_hints.utils.centroid_outfacing_mesh"  (wrong-import-order)
atlas_placement_hints/layered_atlas.py:39:0: C0412: Imports from package trimesh are not grouped (ungrouped-imports)
atlas_placement_hints/layered_atlas.py:40:0: C0412: Imports from package voxcell are not grouped (ungrouped-imports)
************* Module atlas_placement_hints.app.placement_hints
atlas_placement_hints/app/placement_hints.py:339:0: R0913: Too many arguments (9/7) (too-many-arguments)
atlas_placement_hints/app/placement_hints.py:405:8: R1722: Consider using 'sys.exit' instead (consider-using-sys-exit)

------------------------------------------------------------------
Your code has been rated at 9.70/10 (previous run: 9.70/10, +0.00)

lint: exit 28 (13.85 seconds) /PATHREPLACEDHERE/> pylint atlas_placement_hints pid=28254
  lint: FAIL code 28 (18.89=setup[3.98]+cmd[0.22,0.22,0.40,0.21,13.85] seconds)
  evaluation failed :( (18.97 seconds)

[asoplata]

I've tried to update tests/app/test_placement_hints.py to match the new workflow but I clearly don't know what I'm doing. The new workflow is described in atlas_placement_hints/layered_atlas.py::ThalamusAtlas. Basically, how the general thalamus testing should go is:

1. Run atlas-placement-hints thalamus with --create-uncut-thalamus-meshes-flag to create meshes, but NOT create placement-hints,
2. Perform the copying in tests/app/test_placement_hints.py::test_thalamus to "simulate" creation of hand-cut meshes, then
3. Run atlas-placement-hints thalamus again, but this time with --load-cut-thalamus-meshes-flag, again checking for an error exit code. This is the step that actually creates the placement-hints.

After step 3 has happened, then there should be a new placement_hints/distance_report.json that can work for the remaining tests. However, when I try to add the most recent code, I get simply an empty AssertionError message. Do I need to invoke a different runner or change something else? Apologies, I'm still relatively new at testing in Python and in general.

FAILED tests/app/test_placement_hints.py::test_thalamus - AssertionError:

[arnaudon]

To fix your tests, you have to modify the ThalamusMock here https://github.com/BlueBrain/atlas-placement-hints/blob/main/tests/placement_hints/mocking_tools.py#L196 so that it matches what you create with the updated code. Its hard to say what is diff without looking in details in what you did, but if you think its good, just adapt this Mock thing, and it should be good.

[asoplata]

Okay, it will take me some time to go through this and update the tests then. I should be able to get to it within a few weeks, got a lot going on

[asoplata]

Please allow the tests to run on the latest commit (don't cancel them please)

[asoplata]

Darn, I was hoping that that small change could fix it. The main problem I'm having at this point is constructing a working test environment in the first place. Unfortunately, I believe I need additional help from someone at NSE like @mgeplf to figure this out. Even when I appear to install a working test venv (including cgal-pybind and ultraliser) on BB5, the tests on both this branch and even the main branch fail, and fail in similar ways. I think this means my issues are something to do with the environment, but I'm not sure. I'm going to try installing the test environment from scratch (on a linux), but I will likely need some help.

[arnaudon]

I can fix your tests a little later today or tomorrow, it does not look like a big deal, don't fight this too much, its not worth it haha

[arnaudon]

ok, @asoplata , tests fixed, you had to also change the region id in the other test file. I also ran format, so you can now see the lint errors in the last CI, you see quite a few due to your change in the main code. I'm not sure if what you did there could be accepted by NSE, you'll have to ping them maybe on slack to see if they have time to check it. I don't have merge rights anyway on this repo.

[arnaudon]

Ah, to bypass cgal thing locally, just comment the cgal line in setup.py before you run tox, and module load it, so you have it for the tests that need it. But don't push the change in setup.py in your commit.

[asoplata]

This should be merge-able now.

[arnaudon]

Did you check my comments above? I added two proper reviewers that have merge rights to double check

[lecriste]

I believe this is a scientific change, don't know how to help here.

[arnaudon]

not the stuff about saving meshes, but I guess if you don't care, you can approve so austin can go ahead and use this version in the workflow

[lecriste]

Which workflow? The PR description reads "This makes significant changes to the creation of placement-hints for the thalamus", before approving and update the placement-hints in the Atlas pipeline I would like some scientific review of these new placement-hints.

[asoplata]

@lecriste What does a scientific review entail? Do you want someone else to approve of the final changes on a scientific level?

[lecriste]

Not necessarily. I'm just saying that before updating the Atlas pipeline to use this new code, it would be good to provide the consumer of placement hints (@eleftherioszisis and @mgeplf AFAIK) with this new version and ask for their feedback.

[arnaudon]

Sorry, I wasn't clear, the science is good, autin is world expert on that, and the result look quite reasonable to me. The only thing I'm wondering is if the option to save thalamus meshes (thalamus_meshes_dir) makes sense. I would not make it thalamus specific, but generic for all regions, but this should be done in another PR. Then, regardless of the scientific validity, I believe this PR should be reviewed anyway (as it has always been the case for NSE codes). Austin made some changes with typing, I'm not sure are the best, and ho also probably this save mesh option, but I'm not sure how to improve it, as I have not worked much on these codes, so I don't know the code quality you expect here.

[lecriste]

Sorry, I wasn't clear, the science is good, autin is world expert on that, and the result look quite reasonable to me.

So it's OK to change the PH of the Thalamus region of these files to the values from this code?

Then, regardless of the scientific validity, I believe this PR should be reviewed anyway (as it has always been the case for NSE codes). Austin made some changes with typing, I'm not sure are the best, and ho also probably this save mesh option, but I'm not sure how to improve it, as I have not worked much on these codes, so I don't know the code quality you expect here.

I know less than you.