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

Prevent a ValueError if the input variable has constant value #33

Closed
wants to merge 4 commits into from

Conversation

yslai
Copy link

@yslai yslai commented Mar 26, 2018

Currently, input variables with a constant value will abort with ValueError: arange: cannot compute length, due to numpy.arange() cannot handling ranges with zero length. This modification adjusts the range min/max such there is a small offset.

@coveralls
Copy link

coveralls commented Mar 26, 2018

Coverage Status

Coverage decreased (-0.4%) to 76.852% when pulling 0984d27 on yslai:master into 77cc110 on natekupp:master.

@natekupp natekupp requested a review from jmmcd March 27, 2018 18:35
@natekupp
Copy link
Owner

@jmmcd mind taking a look?

@yslai
Copy link
Author

yslai commented Mar 28, 2018

The previous PR did not properly handle magnitudes far away from 1. This revised version dynamically sets rangex according to the floating point property. Also, minx and maxx are now scaled to properly recover len(thrs) == ss.num_thrs_per_var + 1 further down in the code.

Copy link
Collaborator

@jmmcd jmmcd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

It just seems a bit convoluted and arbitrary. And most important, none of the bases we create on this variable will be useful, no matter what hack we try!

So how about at line 654, we calculate minx and maxx, and if minx == maxx, then we just continue, ie don't create any bases for this variable?

@pizzooid
Copy link

In #36 (detail) i also had to include a fix for this issue but i think that @jmmcd is right a new threshold for a variable with a constant value is unnecessary and should call continue.

jmmcd added a commit to jmmcd/ffx that referenced this pull request Dec 13, 2019
natekupp pushed a commit that referenced this pull request Dec 18, 2019
* Bump version number for PyPI release, and include Py3.6 as supported.

* Update push_to_pypi to use twine.

* Update runffx tool for Python 3

* Added a Scikit-learn API, bumped FFX version number and Python/Sklearn numbers.

* Squeeze an unused dimension in y. Move a comment in api. Add a commented line for pushing to test-PyPI.

* Bump version number again and remember to rm old versions when uploading to PyPI.

* Corrected the count of GP nodes (complexity) and used it in existing test, also added the code from the README as a test.

* Bump version number for PyPI

* Fix special case for thresholds when x var has no variance.

* Change _model to model_ and same for _models to conform to sklearn Estimator API. Add FFXModel.predict as alias of FFXModel.simulate so individual models can be used in pseudo-sklearn model.predict(X) style.

* Instead of hacking thresholds for case where x var has no variance, just continue. See #33.

* Make FFXModel and ConstantModel into sklearn Regressors and add predict to the latter also, and add a test of all this.
@natekupp
Copy link
Owner

natekupp commented Jul 4, 2021

closing this as stale, but please re-open if this is still an issue

@natekupp natekupp closed this Jul 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants