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

Change defaults for global signal regression and minimum image regression #161

Closed
tsalo opened this issue Nov 28, 2018 · 6 comments
Closed
Labels
discussion issues that still need to be discussed
Milestone

Comments

@tsalo
Copy link
Member

tsalo commented Nov 28, 2018

We have discussed changing the defaults for global signal regression and T1c-GSR here, but I think it would be good to address this in its own new issue.

I propose that we do the following:

  1. Change default for GSR from running it by default to not running it. We can do this by changing the argument --no_gscontrol to --gscontrol.
  2. Add argument to select a global noise removal method. This is implemented in [WIP, ENH] Add GODEC #120 as --ws_denoise, but that PR is not going to be merged any time soon, and I'd like to change these defaults sooner than later. Perhaps --global_denoise or --postproc would also be good names? Ultimately, we plan to support multiple options for this, including, at minimum, T1c-GSR and GODEC, so it would be better to plan for that now. The default for this argument would be None, and to turn T1c-GSR on we could use t1c, mir, or something else entirely.

Does anyone have any issues with turning off GSR and T1c-GSR (i.e., minimum image regression) by default?

@tsalo tsalo added the discussion issues that still need to be discussed label Nov 28, 2018
@tsalo tsalo added this to the 0.1.0 milestone Nov 28, 2018
@handwerkerd
Copy link
Member

No issues with turning this off.

@emdupre
Copy link
Member

emdupre commented Nov 28, 2018

To be clear, I think we are not running gs control by default -- see

parser.add_argument('--no_gscontrol',

Am I misunderstanding this ?

@tsalo
Copy link
Member Author

tsalo commented Nov 28, 2018

We are running gs control by default, since the argument --no_gscontrol turns it off. The CLI and the workflow are a little weird. The CLI flag --no_gscontrol sets the workflow function argument gscontrol to False, but the default is True.

If anything, changing --no_gscontrol to --gscontrol in the CLI (and changing the default in the function from True to False) will make everything easier to follow.

@tsalo
Copy link
Member Author

tsalo commented Dec 2, 2018

Perhaps one argument against --global_denoise is that we don't want to have a submodule called global (for Python reasons) or denoise (tedana reasons). An argument against --postproc is that that doesn't include the standard GSR that is run on the input data rather than the denoised data, although maybe we don't want to roll GSR in with GODEC and T1c-GSR. Does anyone have any ideas for a better name?

@emdupre
Copy link
Member

emdupre commented Dec 3, 2018

Ok, for some reason I remember us changing the default because we had turned it off, but I'm having trouble tracking down that history right now. I'm very pro removing it.

@tsalo
Copy link
Member Author

tsalo commented Dec 4, 2018

Closed by #163.

@tsalo tsalo closed this as completed Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion issues that still need to be discussed
Projects
None yet
Development

No branches or pull requests

3 participants