Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Update for ultraliser> 0.4 #18

Merged
merged 4 commits into from
Feb 1, 2024
Merged

Feat: Update for ultraliser> 0.4 #18

merged 4 commits into from
Feb 1, 2024

Conversation

arnaudon
Copy link
Contributor

@arnaudon arnaudon commented Jan 29, 2024

This works with ultraliser==0.4.1, which will appear on spack shortly BlueBrain/spack@567c676

@arnaudon arnaudon requested a review from mgeplf January 29, 2024 15:04
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #18   +/-   ##
=======================================
  Coverage        ?   79.91%           
=======================================
  Files           ?        9           
  Lines           ?      453           
  Branches        ?        0           
=======================================
  Hits            ?      362           
  Misses          ?       91           
  Partials        ?        0           
Flag Coverage Δ
pytest 79.91% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@arnaudon arnaudon requested a review from lecriste January 29, 2024 15:28
@mgeplf
Copy link
Collaborator

mgeplf commented Jan 30, 2024

@arnaudon Does ultraliser 0.4.0 produce reasonable and better results?

@arnaudon
Copy link
Contributor Author

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
Copy link
Collaborator

mgeplf commented Jan 30, 2024

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
Copy link
Contributor Author

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
Copy link

lecriste commented Jan 30, 2024

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 arnaudon mentioned this pull request Feb 1, 2024
@arnaudon
Copy link
Contributor Author

arnaudon commented Feb 1, 2024

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
Copy link
Contributor

asoplata commented Feb 1, 2024

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 #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
Copy link
Contributor Author

arnaudon commented Feb 1, 2024

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

Copy link

@lecriste lecriste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No impact on the current Atlas pipeline:
#18 (comment)

@mgeplf mgeplf merged commit 317a1f2 into main Feb 1, 2024
5 checks passed
@mgeplf mgeplf deleted the ultraliser_0.4.1 branch February 1, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants