Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Bspline #39
Bspline #39
Changes from 11 commits
d31fc86
0a69205
13cc208
7c9c6df
9900124
cf33067
5680b45
42f846f
097bcdd
1767250
e3384f9
fa582aa
6a02fce
4b9c246
9fd5f9d
a8f75b5
378d890
2949d85
4103632
f988b1a
efb3c6a
ba815cc
0e8b0e0
60ef87a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this here and only for BSpline?
from the tests, looks like you're expecting it when
evaluate_on_grid(x)
forx<=0
, but is that really an issue? for the negative case, we'll get an error innp.linspace
and forx=0
, you'd get an empty array otherwise, which seems reasonable. Is there a specific reason to check for this?and in general, don't like that we'll have different behavior for BSpline than for the rest, when really, it's the same concerns, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't check, with x=0 you get an "invalid input data" error message from the scipy splev call.
I am fine with that but:
Uniformity in behavior is hard to get given that different basis construction have different requirements.
My rationale here was try to surface the error and give a basis-type specific explanation of the error which is understandable.
If we try to get something that looks more uniform there are a few way:
Which one do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. I'm inclined to work towards uniformity in behavior, and I don't have a strong preference here, but I think raising the error for all of them is the right way? because if someone calls
evaluate_on_grid(0)
, they've really misunderstood what it does. right?