-
Notifications
You must be signed in to change notification settings - Fork 14
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
Template-based segmentation, and new templates (marmoset, macaque) redo #281
Conversation
note: this is just a copy of the marmunfold branch
btw, I'm going to generate dseg & coords on the CITI template (just running through hippunfold), so we can use that for template-based segmentation as well (as long as the dseg looks good).. |
- in CITI168 we have L and R masks.. - but will need to adjust this to get it working for data (ie Marm ex vivo) where only a single hemi exists -- will need to include some logic to decide when to register L hemi, vs when to register Lflip..
- checks config to see what template hemis are enabled, uses flipping for other hemis
ok this is good to test out now, I've enabled templateseg for MBMv3 and CITI168 templates |
My tests still won't run end-to-end. Specifically, the template injections steps in the Will look into it more next week |
Ah weird - what were your input args? I'll try from my end too. I tested templateseg with mbmv3 and citi168 and was working fine.
…________________________________
From: Jordan DeKraker ***@***.***>
Sent: Wednesday, February 21, 2024 6:26:25 PM
To: khanlab/hippunfold ***@***.***>
Cc: Ali Khan ***@***.***>; Author ***@***.***>
Subject: Re: [khanlab/hippunfold] Template-based segmentation, and new templates (marmoset, macaque) redo (PR #281)
My tests still won't run end-to-end. Specifically, the template injections steps in the hemi-L are generating volumes containing all 0. If I apply the injection warp myself via ANTs or if i shell into the container and warp with greedy then it works fine, but it won't work within the pipeline for some reason.
Will look into it more next week
—
Reply to this email directly, view it on GitHub<#281 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACXV2XJB5WRTKGAEIQUH4ULYUZ7CDAVCNFSM6AAAAABDMJHZISVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJYGMYTSNBWGI>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
the input is one from the closed dataset that Nicole used in her project |
Ah OK- I'll try it out on an openly available macaque dataset tomorrow. Could be the template voxel spacing, as I set that with cli when using marm template. It's something we could pre-configure if so... |
Looks like that is actually an ex vivo macaque template -- I don't have an ex vivo macaque dataset to try it out on, and it might have limited utility for in vivo data (the couple scans I tried failed linear registration).. I am assuming the dataset you are applying it to is an ex vivo scan? |
- ensures entries in template_based_segmentation are correct (ie only include templates that have coords/dseg, and whatever hemis are available
ahh haha that perfectly explains the error i was getting - when i applied the segmentation manually i of course used the correct template file! |
My test case worked, I think this is ready to merge save for the Docs error... |
Great! We could merge this in then and do doc updates in another PR |
* interpreting QC (#279) Co-authored-by: Jordan DeKraker - B. Bernhardt Lab <[email protected]> * Bump version to 1.4.2-pre.36 * lock requirements versions * format * applied similar logic to procT1w * linting --------- Co-authored-by: Jordan DeKraker - B. Bernhardt Lab <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
I see the recent flurry of activity @jordandekraker ! Would be great to resolve this PR too -- let me have a look to see what is failing the tests, I think we are good to merge after that is done, right? |
Haha yes, just got back from a conference and wanted to clean things up a bit. I forget the issue that I was having here, but i have no uncommitted changes or notes. The soonest I can get to it will probably be Friday |
was causing errors in the new download rules
the Patch T2w PR merge into this broke some of the code that was updated in this branch to deal with downloads properly.. this reverts it back to the previous version.. With the new download rules, the template folder is put as an input, and the template files themselves as params.. But with this rule, the use of a template is optional (depending on t1_reg_template), so we need to use a function to define the command.
no longer needed since the models are all downloaded now..
Sorted out what was causing the tests to fail - the edits to preproc_t2.smk you made in #288 overrode some required changes in this branch related to downloading template files.. Not sure how that happened but could be because of that branch was based off something else.. Anyways, probably worth giving this a "wet"-run before we merge in, I'll run something now |
My "wet"-run succeeded! I also have a local commit to add the Allen Mouse Brain. The only changes are in snakebids.yml to provide the new template and better descriptions. The OSF link is still blank since I wasn't sure whether to make this public yet. I did notice that the ABA template fails in some other mouse brains still (its very detailed and typical scans don't have this level of contrast). so the ABA template, and probably the macaque and marmoset templates, could probably all use a bit of benchmarking. Maybe we can meet soon along with @Bradley-Karat to discuss? |
Let's leave the mouse template out of this PR, it already has alot! - but given that it is not an MRI template I think it might need some more dedicated attention in a separate PR.. Also planning to make a PR to have hippunfold work on some new bids-microscopy datasets I have with lightsheet imaging on mice.. |
This reverts commit f2039f4.
OK should be ready for merge! |
great! I'll merge it in now.. note, before we do a versioned release, we should have something to deal with the fact that the workflow now requires internet -- e.g. have a |
- Template-based segmentation, and new templates (marmoset, macaque) - Downloads external resources (models, templates, atlases) instead of relying on local resources folder
This is a clean version of the #264 PR. Should do some testing before this is merged in. Will need some doc updates too!
(messed up my other branch with a rebase, so this is a new PR)
TODO: add any relevant info that is in the other PR comments..