Update thalamus #11
Update thalamus #11
Conversation
|
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:
@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. |
|
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. |
Load thalamus meshes from CLI input, clean CLI
asoplata
force-pushed
the
update_thalamus
branch
from
b2090d2 to
d183a67
Compare
Codecov ReportAttention:
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This updates the regular expression used for thalamus placement-hints to be in a different format that has been tested successfully, excludes habenular and peripeduncular subregions, and to be valid for the hierarchy/annotation used at its appropriate step in the Atlas pipeline. For information on which regions were chosen and this list was created, see the internal BBP Confluence page located at "Circuits > Mouse Thalamus > Atlas-based Whole-thalamus subregion selection". This regex has been built from the region list of the desired and present thalamus regions as of the "final" version of the hierarchy and annotation built by the Atlas pipeline, which is the output of the rule `split_barrel_ccfv3_l23split`. This change is meant to go in tandem with BlueBrain/atlas-direction-vectors#27 .
|
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. |
|
run |
|
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 |
|
I forgot to mention, to repro the error in the test, just do |
|
Is there a particular configuration of BB5 modules I can use to build the test environment that In order to build the 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 |
|
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 |
|
I've made the changes needed for passing |
|
I've tried to update
After step 3 has happened, then there should be a new |
| has_hemispheres=True, | ||
| ) | ||
| else: | ||
| print( |
There was a problem hiding this comment.
don't print in these cases, raise an Exception as the code will not work, its better to raise a clear exception, the user may not read the log properly, and get into another bug, harder to understand
|
To fix your tests, you have to modify the |
|
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 |
|
Please allow the tests to run on the latest commit (don't cancel them please) |
|
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. |
|
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 |
|
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. |
|
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. |
This does a lot of small things for passing the linting. For mypy, I had to add additional ignores for the Trimesh returned types, since the ignore on the module as a whole wasn't preventing mypy from expecting `load_mesh` to return a Geometry object, which is a grandparent of Trimesh objects. I don't know if Trimesh changed their API, I couldn't figure it out from the docs, and I don't know why mypy was raising this now. In all the cases I could test or see, a proper "Trimesh" object was returned instead of the more generic Geometry. I don't think we need to worry about this. For the pylint disable W0613 (unused-argument), I needed some polymorphism for the thalamus case, but I wasn't sure how to handle that alongside the linters' type-checking. I think this is the simplest solution and is harmless. Everything else is minor.
|
This should be merge-able now. |
|
Did you check my comments above? I added two proper reviewers that have merge rights to double check |
|
I believe this is a scientific change, don't know how to help here. |
|
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 |
|
Which workflow? |
|
@lecriste What does a scientific review entail? Do you want someone else to approve of the final changes on a scientific level? |
|
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. |
|
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 ( |
So it's OK to change the PH of the Thalamus region of these files to the values from this code?
I know less than you. |
|
|
||
| \b | ||
| - `[PH]y.nrrd` | ||
| - `[PH]Rt.nrrd`, `[PH]VPL.nrrd` | ||
| - `[PH]layer_1.nrrd` (This is the "top-most" layer, equivalent to "RT". | ||
| This previously used the filename `[PH]RT.nrrd`) |
There was a problem hiding this comment.
is there a reason you're calling it "layer 1" instead of RT? I find the later easier to reason about
There was a problem hiding this comment.
Yes, it's a new requirement imposed by the BBP Atlas Pipeline: for any rules that apply region-specific customization (such as placement hints for the thalamus), the output files need to be named in a very particular way. If you have questions, slack me or look at the "Customize a pipeline rule" section of the BBP Atlas Pipeline README.
(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
ThalamusAtlasclass.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.