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

Consider checking / changing default values #303

Open
TomDonoghue opened this issue Sep 12, 2023 · 1 comment
Open

Consider checking / changing default values #303

TomDonoghue opened this issue Sep 12, 2023 · 1 comment
Labels
2.0 Targetted for the specparam 2.0 release. idea / discussion A potential idea to consider / discuss.

Comments

@TomDonoghue
Copy link
Member

TomDonoghue commented Sep 12, 2023

With the move to specparam / 2.0 it's a good time to consider, and potentially update, the default settings we use in the module.

Since 1.0 (at least, though perhaps much before that), we have defaulted to the following public settings:

  • peak_width_limits=(0.5, 12.0)
  • max_n_peaks=np.inf
  • min_peak_height=0.0
  • peak_threshold=2.0
  • aperiodic_mode='fixed'

Is there anything here we could / should tweak at all?

For example, there is some discussion of potentially defaulting to a non-infinite value for max_n_peaks (#299 (comment)), related to how fitting way too many peaks (often in an undesirably way) can really slow things down. Right now, we default to no bounds, and we could update to some number - though what that number should be is pretty tricky. I generally recommend setting this value to twice the number of real peaks you might expect (since, with Gaussians, we can get two gaussians per peak in the asymmetric case), which would tend to lead to a max of around 8 - but this is quite dependent on the frequency range / context, etc - so it's not clear if we can pick a good number. One possibility is to pick a probably too big number (say, 12), to at least default to pushing away from the cases where we can accidentally fit dozens of peaks (I think I would probably vote for this).

The other unbound parameter that is something important to bound is the absolute threshold (min_peak_height), but without knowing the scale of the data, I don't see a way to default this.

I don't see any reason / alternative for changing peak_width_limits or peak_threshold - but very open to hearing any thoughts on that. Note that the mode setting will be updated by the new approach in 2.0, so we don't have to discuss that here.

Notably, an additional point here is whether we want to increase the emphasis on choosing settings (say, in the docs).

@TomDonoghue TomDonoghue added 2.0 Targetted for the specparam 2.0 release. idea / discussion A potential idea to consider / discuss. labels Sep 12, 2023
@rdgao
Copy link
Contributor

rdgao commented Sep 12, 2023

just following up @voytek's comment: I agree that restricting defaults to neuro-specific settings might be less optimal now that there are users from a broad range of fields.

At the same time, @TomDonoghue's recommendations from above are exactly the type of insight that people ask for after using it for some time, so maybe defaulting to 10 or so would be a good compromise (side note: I almost always recommend n_max_peaks=3). The other option is just to not have a default so it forces you to think about it, and the values from the tutorials would essentially serve as a default already.

min_peak_height could similarly have a very liberal default value, i.e., very small, but just big enough that it's not 0? I feel like with this and a non-infinite n_max_peaks, it would help speed-up >80% of use cases but not suffer any accuracy loss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 Targetted for the specparam 2.0 release. idea / discussion A potential idea to consider / discuss.
Projects
None yet
Development

No branches or pull requests

2 participants