improve error message for cost function #863
Conversation
|
Cool, thanks! I think the error message is spot on. Would you be willing to also write a test for this? Otherwise I would merge as is and write a test at some later point. Ideally we want to maintain 100 % test coverage. A test would go to |
|
Thinking more about it, there are a couple of things that need to be improved.
|
|
Hi @HDembinski Thanks for the feedback and suggesstions. |
|
Hi @HDembinski I have added a test for raisevalue error using pytest.raises. However this test fails since scipy is not installed. so shall I include the above statement to skip test if scipy is not available? I have also implemented a check for shape compatibility. Please let me know if something needs to be changed. Thanks, |
Yes, please. This is happening because scipy is an optional dependency in some methods. Those methods work with and without scipy being installed, but execute different code in both cases. We want to test both code paths, that's why the tests are run with and without scipy. When you test a method for which scipy is required, you need to explicitly mark so that it is skipped in those test runs. |
amanmdesai
force-pushed
the
imrpoved-error-message
branch
from
ad2d953
to
fdb93da
Compare
I see. Thanks. I have added that statment. |
src/iminuit/cost.py
Outdated
| if len(self._xe_shape) != len(self._model_xe): | ||
| raise ValueError("Variable shapes do not match") |
There was a problem hiding this comment.
I think this always matches by construction, no? So this if should never trigger and can be removed.
src/iminuit/cost.py
Outdated
| if len(self._data) != len(d): | ||
| raise ValueError( | ||
| f"Expected model to return an array of size {len(self._data)}," | ||
| f"but it returns an array of size {len(d)}" | ||
| ) |
There was a problem hiding this comment.
Good, but d and self._data could be multidimensional, so we want to check self._data.shape == d.shape.
There was a problem hiding this comment.
The error message needs to be adapted accordingly, "array of size" -> "array with shape" etc
tests/test_cost.py
Outdated
| @pytest.mark.skipif(not scipy_available, reason="scipy.stats is needed") | ||
| def test_error_message_cost(): | ||
| from iminuit import cost | ||
| import numpy as np | ||
| from scipy.stats import norm |
There was a problem hiding this comment.
Ah, this test only requires scipy because you import norm from scipy.stats. That's not necessary. We don't actually want to fit this cost function, so we can invent a bogus model that just returns a constant array with the wrong shape. In other words, you don't need norm to test this.
There was a problem hiding this comment.
Please also test the multi-dimensional case, where edges is a tuple of two arrays, with edges for the x and y-axis, and n is a 2d array.
|
Hi @HDembinski I have implemented all suggesstions with the exception of 2D test (I could not implement this test). |
Ok, I implemented the 2D test and found some issue with our implementation. We should already test the length of the array returned by the model before it is further reshaped by I made a couple of unrelated changes, to make mypy happier and to increase coverage back to 100 %. |
Thanks a lot @HDembinski for your help and guidance! |
|
Thanks @amanmdesai ! |
Hello @HDembinski
This PR is regarding #791