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

Add options to presets.ThresholdingEnsemble. #1148

Merged
merged 1 commit into from
Aug 25, 2016

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented Aug 24, 2016

Description:
Adds two more options to the thresholding preset to control the width of the intercepts and the radius of the thresholding ensembles.

Motivation and context:
Sometimes it can be useful to fine tune these parameters.

Interactions with other PRs:
none

How has this been tested?
The existing test for the preset still passes.

Where should a reviewer start?
The usual place.

How long should this take to review?

  • Quick (less than 40 lines changed or changes are straightforward)

Types of changes:

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Still to do:

@Seanny123
Copy link
Contributor

LGTM. I don't think this needs a changelog entry. I'm on the fence about whether we need a test to see if changing the radius actually works, but I'm leaning towards no, since it's really already covered by ensemble and config tests.

@jgosmann
Copy link
Collaborator Author

Actually, I should write a test and make sure that this works as intended. I just remembered that eval_points get scaled by the radius (intercepts maybe too?).

@jgosmann jgosmann removed their assignment Aug 24, 2016
@jgosmann
Copy link
Collaborator Author

Regarding the changelog entry: I just added this PR to the existing entry. There wasn't a release wtih the preset yet.

@tbekolay
Copy link
Member

Documenting presets is the last thing on my todo for 2.2.0, so this should be in before 2.2.0 too. Pushed a fixup to add the defaults to the docstring. LGTM! Will merge once tests finish.

Namely, intercept_width and radius to allow for more fine tuning.
@tbekolay tbekolay merged commit 0063130 into master Aug 25, 2016
@tbekolay tbekolay deleted the thresholding-preset-options branch August 25, 2016 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants