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

Fixes issue 295 #296

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fixes issue 295 #296

wants to merge 7 commits into from

Conversation

Jhsmit
Copy link
Collaborator

@Jhsmit Jhsmit commented Feb 6, 2020

Checks if any bounds are none before comparing min/min

For some reason there are some commits listed which are already on symfit master

Comment on lines 233 to 237
if min is None or max is None:
pass
elif min > max:
raise ValueError('The value of `min` should be less than or'
' equal to the value of `max`.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you condense these conditions? min is not None and max is not None and min > max?
Alternatively, completely remove the check here, and make min/max a property of Parameter, and do the checking in the setter. That should clean up some logic around the place.

Removed min/max check because its already in ``Parameter``'s init
@Jhsmit
Copy link
Collaborator Author

Jhsmit commented Feb 6, 2020

Edited on GitHub which also added a newline at the end of the file

@pckroon
Copy link
Collaborator

pckroon commented Feb 6, 2020

Ok, but then you also need to add the Parameter.min and Parameter.max properties... Currently we'd just lose a useful check instead of moving it.

@Jhsmit
Copy link
Collaborator Author

Jhsmit commented Feb 6, 2020

The current check I've removed was redundant because its also in the __init__ of Parameter, where also the possibility of None is checked in one line.

In any case, the getter/setter would still add functionality because it prevents users from setting faulty limits after initialization of the parameters.

@pckroon
Copy link
Collaborator

pckroon commented Feb 7, 2020

parameters does not call Parameter.__init__. Instead it does for param, value in zip(params, values): (param, key, value) (https://github.com/tBuLi/symfit/pull/296/files#diff-f73c0cdf32475d7606d6d487cae94340R245).

Of course, once there's setter methods the check in Parameter.__init__ will become redundant.

@Jhsmit
Copy link
Collaborator Author

Jhsmit commented Feb 7, 2020

Ah I see, Parameter.__init__ does get called but not with the user supplied min/max values which indeed are set later.
I'll put in the setter methods instead.

@tBuLi
Copy link
Owner

tBuLi commented Mar 19, 2020

Yeah I agree that putting this check in the setter is probably a nicer solution. Another solution could be to initiate each object properly and not set it after the fact.

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.

3 participants