-
Notifications
You must be signed in to change notification settings - Fork 94
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
Adding robustica option to ICA decomposition to achieve consistent results #1013
base: main
Are you sure you want to change the base?
Changes from 19 commits
f4eaa3e
b0cac3a
55c2ae4
cd55a3f
2d9b007
09e565e
fc5f9ea
4fc3043
a20ff57
cc5e05d
a449fec
78c8140
ac85e6a
979d026
71d8d4a
cac38cd
5fcf148
b7d08e9
a113423
b60e9a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
"""Setting default values for ICA decomposition.""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd lean against making this config file. Anyone who installs I'd lean towards keeping our current method of setting defaults with input options so it's more clear to the end-user what the used options are. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm noticing that the config file variables get to be used in both the Argument Parser and the variable defaults for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're concerned about defaults not matching across functions, we can just make all parameters require the parameter name (e.g., There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without necessarily pushing for one solution or the other code-structure-wise, I would say that perhaps if this kind of solution is used, perhaps Also, while removing defaults from function arguments would reduce the problem, it might not remove it completely. Sometimes for a command-line option, you want to be able to state what the default behaviour or value will be within the help string. While it's easy to cross-reference within the |
||
|
||
DEFAULT_ICA_METHOD = "robustica" | ||
DEFAULT_N_ROBUST_RUNS = 30 | ||
DEFAULT_N_MAX_ITER = 500 | ||
DEFAULT_N_MAX_RESTART = 10 | ||
DEFAULT_SEED = 42 | ||
|
||
|
||
"""Setting extreme values for number of robust runs.""" | ||
|
||
MIN_N_ROBUST_RUNS = 5 | ||
MAX_N_ROBUST_RUNS = 500 | ||
WARN_N_ROBUST_RUNS = 200 | ||
|
||
|
||
"""Setting the warning threshold for the index quality.""" | ||
|
||
WARN_IQ = 0.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this dependency makes a lot of sense, but I want to highlight to @tsalo & others that this will also install two additional dependencies: scikit-learn-extra & https://tqdm.github.io/
We've had issues with adding modestly supported modules in the past (😱duecredit 😱) so we'll want to keep an eye on all three of these, particularly in relation to #934.
Also
tqdm
is a progress bar module. If we're going to require it, we might want to think if there are others parts of the code where it might be useful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there's a problem with
scikit-learn-extra
in the Python 3.12 tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an opinion here - I had a branch where I played with putting the progress bar in for
curvefit
(I think) cause it is slow and can be confusing when it just sits there - I think it worked nicely but got distracted. Once this is pulled in, I'll look into doing that again. I don't think it makes sense for loglin, but could anyway (being forward looking, maybe it will be slow someday on some data?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that idea. I've used tqdm before without issue (e.g., in NiMARE), so I'm all for adding progress bars. Not sure about applicability to loglin though. Don't we only loop over unique adaptive mask values there, rather than voxels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point - yeah, makes no sense there. I hadn't even checked it. Curve fit can use it, maybe figure creation? Honestly, once it punches through the ICA steps its hustling so may not be needed elsewhere.