Changes basis inheritance of evaluate / evaluate_on_grid

[billbrod]

This PR is an implementation of #30 : it changes where the public instantiation of the basis evaluate / evaluate_on_grid happens so that the user-facing docstring and function signature are more transparent.

Previously, the docstring that showed up for these two methods was the one from the super class, which was generic and suggested that they could accept an arbitrary number of inputs, but e.g., the following:

bs = nsl.basis.MSplineBasis()
bs.evaluate_on_grid(5,5)

fails. Now, the docstring for evaluate and evaluate_on_grid make it clear that they expect a single input.

This required renaming the superclass evaluate to _check_evaluate_input, and then all the subclasses call that method first.

The superclass evaluate_on_grid method is still public, and it's the one used directly by AdditiveBasis and MultiplicativeBasis

Also:

• Did some minor cleaning up of the docstrings.
• Made some changes for what we check for in evaluate input. We no longer test the type of sample_pts directly, instead we see if it can be cast to a numpy array with floating point type, of at least 1d.

I'm not super happy about:

• AdditiveBasis and MultiplicativeBasis still have the less informative docstring. Given their recursive nature, I'm not sure there's a way around this without doing something real weird like setting self.evaluate.__func__.__doc__ during initialization, which seems ... bad
• There's a lot of repetition of evaluate_on_grid: whereas every concrete class had an _evaluate method that just got promoted to evaluate, none of them had anything like that for evaluate_on_grid. So now each has a new evaluate_on_grid whose entire point is just to define the docstring -- the only thing it does is call super().evaluate_on_grid(n_samples). I still think this is better than previously, because the docstring and function signature are clear, but ... it's ugly.

I also think the new docstrings can probably be made more informative. Happy to go and do that if people think this is worth doing.

[billbrod]

Failing tests are due to two reasons:

• Changes in exception raised when the wrong number of inputs are passed (was custom ValueError, now standard TypeError because it gets the wrong number of positional arguments)
• Now evaluate can accept a single float/int (which it casts to a 1d array).

Assuming everyone's okay with these changes, I'll update tests accordingly.

The main question is -- do we want evaluate to accept a single float/int? Given that it could already accept evaluate([.5]), I figured evaluate(.5) was fine.

The argument against it is that it's possible that if somebody called evaluate(5), they actually meant to do evaluate_on_grid(5) and will be confused by the output. I still think it's worth doing this way, but wanted to see what others thought.

[BalzaniEdoardo]

hei Billy,
Great job and thanks for this. I think all changes are well thought out. The problem with evaluate on grid is that all it does is generating the grid and calling the recursion, so there is literally no sub-class specific calculation either than calling self.evaluate at the end of the recursion. I think this is the best we can do, if we aim for informative docstrings.

I do think this is the right choice, the code gets slightly longer but the evaluate is much more clear with informative docstrings.

I would say you should go ahead and fix the tests!
Edo

[BalzaniEdoardo]

I already commented in the response section. I think all the chages are sensible and I think this is the best we can do if we want to have informative docstrings in the evaluation methods. I really like how the code looks now for this module, very satisfied.

Thanks for working this out! You can go ahead with the tests