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 bounds, scale to ApproxPosterior object? #44

Closed
dflemin3 opened this issue May 13, 2019 · 1 comment
Closed

Add bounds, scale to ApproxPosterior object? #44

dflemin3 opened this issue May 13, 2019 · 1 comment
Assignees

Comments

@dflemin3
Copy link
Owner

It could be useful, and make the code cleaner, to have both bounds and scale be initialized with the ApproxPosterior object. Bounds is used in numerous places to maintain numerically stability and validate point selection. Scaling parameters is used throughout now, as well, so it could make sense to initialize the scaler earlier on. Also, this could allow for the user to select several common scales, like MinMax, Normal, etc, using the sklearn interface. MinMax is easy to initialize with bounds for training the scaler, but Normal and other types of scaling requires fitting on training data, which in our case, could be the initial theta.

I can get around breaking the API by allowing the user to override bounds in the ApproxPosterior run method and with sensible default choices for both scale and bounds.

@dflemin3
Copy link
Owner Author

I added bounds to the AP initialization as a required argument and nixed scaling since it didn't obviously help performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant