BlueBrain / atlas-placement-hints

Tools to compute placement hints in the context of brain atlases.
Apache License 2.0
1 stars 2 forks source link

Feat: Update for ultraliser> 0.4 #18

Closed arnaudon closed 7 months ago

arnaudon commented 7 months ago

This works with ultraliser==0.4.1, which will appear on spack shortly https://github.com/BlueBrain/spack/commit/567c676fb2d5dd794244e4cf2555244215135fef

codecov-commenter commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

:exclamation: No coverage uploaded for pull request base (main@337623b). Click here to learn what that means.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #18 +/- ## ======================================= Coverage ? 79.91% ======================================= Files ? 9 Lines ? 453 Branches ? 0 ======================================= Hits ? 362 Misses ? 91 Partials ? 0 ``` | [Flag](https://app.codecov.io/gh/BlueBrain/atlas-placement-hints/pull/18/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/18/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain) | `79.91% <0.00%> (?)` | | 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.

mgeplf commented 7 months ago

@arnaudon Does ultraliser 0.4.0 produce reasonable and better results?

arnaudon commented 7 months ago

I tried on cerebellum, and it looked reasonable with this setup. If I were to decide, I would only use a simple marching cube, I'm not sure about the added value to have sub-voxel resolution here, but I don't know the rest of the algo enough. Are there more test one could do to validate this version? It does not seem tested here. Maybe @asoplata could also try it on his atlas to see? I will keep working on cerebellum atlas in the coming days to better validate, we can keep this open for now, np!

mgeplf commented 7 months ago

I tried on cerebellum, and it looked reasonable with this setup.

Well, that's already a big sign.

Are there more test one could do to validate this version?

Unfortunately I'm not aware of any, so I'm not sure what we can do.

It does not seem tested here. Maybe @asoplata could also try it on his atlas to see? I will keep working on cerebellum atlas in the coming days to better validate, we can keep this open for now, np!

Ok, that's fine with me. I'm also fine with merging it if you need it.

arnaudon commented 7 months ago

this will be breaking change, not compat with <4 (at least not with 0.1.0 that was used before). So maybe I can make sure it still works with both? I'm not sure how this will impact the atlas pipeline, etc...

lecriste commented 7 months ago

There is no validation for placement hints (AFAIK) so the only way to test the impact on the Atlas pipeline is to try it :) I would first generate the current set of placement hints (isocortex only) with

bbp-atlas  --user-config-file customize_pipeline/user_config.json  --snakemake-options '--cores 1'

with target_rule: "placement_hints" in customize_pipeline/user_config.json, and then install this branch and run

  1. again the last command executed by the CLI above (atlas-placement-hints isocortex ...) but with a different --output-dir to make sure the new placement hints are "equivalent" to the previous ones,
  2. your new atlas-placement-hints cerebellum ...
arnaudon commented 7 months ago

If we make a new major release, would it be ok? I'm not sure I can make this work with all the nexus things underneath

asoplata commented 7 months ago

I believe I only tested the thalamus placement-hints using the newer ultraliser v0.4.0, and never successfully used an older version, so it should work - this was my original reason for https://github.com/BlueBrain/atlas-placement-hints/pull/9 .

This also should not affect the current Atlas pipeline, since as Leo said, it only runs the isocortex, and to my knowledge, the isocortex placement-hints algorithm doesn't use ultraliser at all (it uses the "voxel-based" instead of "mesh-based" method).

arnaudon commented 7 months ago

Ah right, then if somebody can approve, I'll merge this and the #19 and #17 which are on top of this one.