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.

Sign up for GitHub

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

Jump to bottom

OPT - map sh to sf before tracking [requires dipy 1.8.0] #782

Merged
merged 4 commits into from
Jan 19, 2024
Merged

OPT - map sh to sf before tracking [requires dipy 1.8.0] #782

merged 4 commits into from
Jan 19, 2024

Conversation

gabknight
Copy link
Contributor

@gabknight gabknight commented Oct 23, 2023 •
edited by arnaudbore
Loading

Quick description

Mapping the SH to SF before tracking speed up det/prob by a factor 2X and PTT by 10X.

requires dipy 1.8.0+

Type of change

Check the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Provide data, screenshots, command line to test (if relevant)

...

Checklist

  • My code follows the style guidelines of this project (run autopep8)
  • I added relevant citations to scripts, modules and functions docstrings and descriptions
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I moved all functions from the script file (except the argparser and main) to scilpy modules
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@pep8speaks
Copy link

pep8speaks commented Oct 23, 2023 •
edited
Loading

Hello @gabknight, Thank you for updating !

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-01-19 19:21:33 UTC

@gabknight gabknight mentioned this pull request Oct 23, 2023
Closed
@arnaudbore arnaudbore self-requested a review October 23, 2023 19:35
@arnaudbore
Copy link
Contributor

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-782/1/

@arnaudbore
Copy link
Contributor

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-782/2/

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

1 similar comment
@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore
Copy link
Contributor

@gabknight can you prepare this PR to be dipy 1.8.0 ready. I will merge it once dipy 1.8.0 is released.

@gabknight gabknight force-pushed the RF_speedup_tracking branch from c558c4b to 48caa59 Compare October 26, 2023 16:10
@arnaudbore
Copy link
Contributor

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-782/5/

@gabknight gabknight changed the title OPT - map sh to sf before tracking OPT - map sh to sf before tracking *requires dipy 1.8.0* Oct 26, 2023
@gabknight gabknight changed the title OPT - map sh to sf before tracking *requires dipy 1.8.0* OPT - map sh to sf before tracking [requires dipy 1.8.0] Oct 26, 2023
@gabknight
Copy link
Contributor Author

This PR is dipy 1.8.0 ready.

@arnaudbore
Copy link
Contributor

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-782/6/

@AntoineTheb AntoineTheb self-requested a review November 20, 2023 14:35
@gabknight gabknight force-pushed the RF_speedup_tracking branch from 48caa59 to c33063d Compare January 12, 2024 21:14
@gabknight
Copy link
Contributor Author

It should be all good. Let see if jenkins finds something.

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

AntoineTheb
AntoineTheb requested changes Jan 15, 2024
View reviewed changes
Copy link
Contributor

@AntoineTheb AntoineTheb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Works really well ! I can track with PTT on ISMRM2015 at npv=30 and step=0.1 in around 6 minutes instead of 1 hour. I only have one small comment about the argparser, the rest LGTM.

Also @arnaudbore @gabknight, should it be on by default ? Does it need that much more ram ?

image

scilpy/tracking/utils.py Outdated Show resolved Hide resolved
@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@AntoineTheb AntoineTheb self-requested a review January 15, 2024 16:08
AntoineTheb
AntoineTheb approved these changes Jan 15, 2024
View reviewed changes
Copy link
Contributor

@AntoineTheb AntoineTheb left a comment

Choose a reason for hiding this comment

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

GTG @arnaudbore

@gabknight gabknight force-pushed the RF_speedup_tracking branch from a4ea6cd to 277aaf7 Compare January 19, 2024 16:23
@arnaudbore
Copy link
Contributor

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-782/9/

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore arnaudbore merged commit 1caf06a into scilus:master Jan 19, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AntoineTheb AntoineTheb AntoineTheb approved these changes

@arnaudbore arnaudbore Awaiting requested review from arnaudbore

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gabknight @pep8speaks @arnaudbore @AntoineTheb