596 unit tests for interstellar.py#601
Conversation
dhuppenkothen
left a comment
dhuppenkothen
left a comment
There was a problem hiding this comment.
Curious why these classes are Nested?
|
I will ask my AI assistant...because I don't understand anything... |
dhuppenkothen
left a comment
dhuppenkothen
left a comment
There was a problem hiding this comment.
It's a great start, but I think your AI assistant might need a bit more practice 😉
| class TestInterstellar: | ||
|
|
||
| class MockInterstellar(Interstellar): | ||
| def attenuation(self, energies): | ||
| return np.exp(-energies) # Mock implementation for testing | ||
|
|
There was a problem hiding this comment.
Curious why these are nested classes?
| return np.exp(-energies) # Mock implementation for testing | ||
|
|
||
| def setup_method(self): | ||
| self.model = self.MockInterstellar() |
There was a problem hiding this comment.
Unless something has changed in PyTest (and it might well have), I'd expect there to be a method called setup_class and anything you'll want to use later on to be stored in cls.model.
| energies = np.array([1., 2., 3.]) | ||
| expected = np.exp(-energies) | ||
| result = self.model.attenuation(energies) | ||
| np.testing.assert_allclose(result, expected, rtol=1e-5) |
There was a problem hiding this comment.
In principle this test makes sense, but the nitpick I'd have with it is that it tests your mock class, which you defined as part of the tests, so it doesn't really test the Interstellar class, does it? Or maybe there's stuff happening in the background I'm not aware of?
| signal = np.array([10., 20., 30.]) | ||
| expected_signal = signal * np.exp(-energies) | ||
| self.model(energies, signal) | ||
| np.testing.assert_array_equal(signal, expected_signal) |
There was a problem hiding this comment.
I'm not sure I understand what this test is doing: signal and expected_signal are both defined in the test, so what the assertion is testing is entirely code inside this test, and not related to X-PSI at all, as far as I can tell.
Does self.model() return anything? I'm assuming that's what you actually want to compare to?
Also, this uses np.testing, and I'm not sure whether numpy's testing framework will clash with pytest. Could this just be an assertion, for example with np.isclose()?
There was a problem hiding this comment.
I think self.model(energies, signal) modifies the "signal" internally as far as I remember. So the signal variable is different after you called that.
| signal = np.array([[10., 15.], [20., 25.], [30., 35.]]) | ||
| expected_signal = signal * np.exp(-energies[:, np.newaxis]) | ||
| self.model(energies, signal) | ||
| np.testing.assert_array_equal(signal, expected_signal) |
There was a problem hiding this comment.
Same issue as test above?
| energies = np.array([1., 2., 3.]) | ||
| signal = [10., 20., 30.] # List instead of ndarray | ||
| with pytest.raises(TypeError, match='Signal must be a numpy.ndarray.'): | ||
| self.model(energies, signal) |
There was a problem hiding this comment.
Do the inputs actually have to be numpy.ndarrays, or could they generically be iterables? Is there a good reason why signal can't be a list? Or a torch.tensor? Or something else that stores vectors?
| energies = np.array([1., 2., 3.]) | ||
| signal = np.array([[10., 15.]]) # Invalid shape | ||
| with pytest.raises(ValueError, match="non-broadcastable output operand with shape"): # with shape (1,) doesn't match the broadcast shape (3,)"): | ||
| self.model(energies, signal) |
There was a problem hiding this comment.
This should fail in two separate ways, that probably should be tested in separate tests. First, here this test is written to fail because the dimensionality of energies doesn't match with the dimensionality of signal . However, I think this should probably also fail because energies is of length 3 and signal is of length 2. Or can you have energies and signals that are of unequal length? I think not, at least not in your mock class. Again, you might run into some trouble here because you're mainly testing the mock class, not actual functionality in the superclass, unless there's stuff I can't see?
| energies = np.array([1., 2., 3.]) | ||
| signal = np.array([[[10., 15.]]]) # Invalid shape | ||
| with pytest.raises(ValueError, match="Invalid number of signal array dimensions"): | ||
| self.model(energies, signal) |
There was a problem hiding this comment.
This test is basically testing the same as the above, as far as I can tell?
3 participants
First attempt at unit tests for the interstellar module.
Do they make sense @dhuppenkothen ?