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

ENH: Adding option to skip randomization of seed. + other fixes #757

Merged
merged 10 commits into from
Nov 9, 2023

Conversation

EmmaRenauld
Copy link
Contributor

@EmmaRenauld EmmaRenauld commented Oct 4, 2023

  • Adding an option to the _dev tracking to have fixed seeds. This allows to create a figure like this:

step_size05

  • Changing the --discard_last_point option to make it the default choice (as discussed with Gab and other; this makes it more like Dipy)

  • Fixing the way the numpy generator and seed are used, as per the numpy doc. If you think it's not the best way to do it, I can just delete the last commit.

@pep8speaks
Copy link

pep8speaks commented Oct 4, 2023

Hello @EmmaRenauld, Thank you for updating !

Line 70:80: E501 line too long (80 > 79 characters)
Line 496:80: E501 line too long (80 > 79 characters)

Line 45:1: W391 blank line at end of file

Comment last updated at 2023-11-07 18:25:28 UTC

@EmmaRenauld EmmaRenauld requested a review from CHrlS98 October 4, 2023 15:44
@arnaudbore arnaudbore self-requested a review October 4, 2023 16:40
@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

Copy link
Contributor

@CHrlS98 CHrlS98 left a comment

Choose a reason for hiding this comment

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

I tested and it seems to work properly.

However, I think this would be more useful if, instead of disabling jittering the seeding positions, we added a --nb_repeats arguments telling how many times we want to track from each seed. The behaviour you want to show could still happen, but it would generalize better and might find other applications than only for teaching purposes?

scripts/scil_compute_local_tracking_dev.py Outdated Show resolved Hide resolved
scripts/scil_compute_local_tracking_dev.py Outdated Show resolved Hide resolved
"REMARK: Our results (our endpoints) seem to differ from dipy's. "
"This should be investigated.")
track_g.add_argument(
"--do_not_randomize_seed_positions", action="store_true",
Copy link
Contributor

Choose a reason for hiding this comment

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

any way we can shorten this param name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed for no_seed_displacement. Ok?

@EmmaRenauld
Copy link
Contributor Author

However, I think this would be more useful if, instead of disabling jittering the seeding positions, we added a --nb_repeats arguments telling how many times we want to track from each seed. The behaviour you want to show could still happen, but it would generalize better and might find other applications than only for teaching purposes?

That is nice, but right now it seems complicated for me. We would have to remember how many times we called get_next_pos.

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@CHrlS98
Copy link
Contributor

CHrlS98 commented Oct 27, 2023

However, I think this would be more useful if, instead of disabling jittering the seeding positions, we added a --nb_repeats arguments telling how many times we want to track from each seed. The behaviour you want to show could still happen, but it would generalize better and might find other applications than only for teaching purposes?

That is nice, but right now it seems complicated for me. We would have to remember how many times we called get_next_pos.

Wouldn't we just need to return the same seed n_repeat times ? This just changes the start position of a streamline, no?

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@EmmaRenauld EmmaRenauld changed the title ENH: Adding option to skip randomization of seed. + other fixes [WIP] ENH: Adding option to skip randomization of seed. + other fixes Nov 7, 2023
@EmmaRenauld EmmaRenauld changed the title [WIP] ENH: Adding option to skip randomization of seed. + other fixes ENH: Adding option to skip randomization of seed. + other fixes Nov 7, 2023
@EmmaRenauld
Copy link
Contributor Author

Should be good!

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

Copy link
Contributor

@CHrlS98 CHrlS98 left a comment

Choose a reason for hiding this comment

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

I would like to validate that the note.txt file in tests is at the right place, but otherwise, everything looks good to me.

scilpy/tracking/tests/notes.txt Outdated Show resolved Hide resolved
@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore arnaudbore merged commit d531467 into scilus:master Nov 9, 2023
@EmmaRenauld EmmaRenauld deleted the option_noRandomSeed branch December 20, 2023 14:48
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.

4 participants