Fix division by 0 in test_multivariate_simulation_2D [no noise] #387
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #387 +/- ##
=======================================
Coverage 98.08% 98.08%
=======================================
Files 22 22
Lines 1147 1147
=======================================
Hits 1125 1125
Misses 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ) | ||
|
|
||
| signal_noise_ratio_hat = np.linalg.norm(y - noise) / np.linalg.norm(noise) | ||
| if signal_noise_ratio != np.inf: |
There was a problem hiding this comment.
Ne we don't want to have np.inf all over the place. We need to regularize the noise norm.
There was a problem hiding this comment.
What do you mean by regularize the noise norm?
There was a problem hiding this comment.
Set it to a non- zero numerical value in any case.
There was a problem hiding this comment.
I don't understand what you mean by non-zero numerical value.
The inf is the case where there is no noise in the signal which seems for me a good value.
There was a problem hiding this comment.
| if signal_noise_ratio != np.inf: | |
| signal_noise_ratio_hat = np.linalg.norm(y - noise) / max(np.linalg.norm(noise), 1e-6) |
I think something like that should solve the division by 0. It will however, not work for the assert_almost_equal test
There was a problem hiding this comment.
I need to have a way to create data without noise.
What is the best way of doing it?
There was a problem hiding this comment.
IMO, the problem comes from returning noise from the simulation. It is redundant with the other outputs (can be obtained by
We could then use the convention SNR=None for a noiseless scenario.
There was a problem hiding this comment.
Yes, but the default value should not be None in that case. It could be 0.
There was a problem hiding this comment.
I prefer a default value of 1.
|
@lionelkusch, can we, for the moment, remove the np.inf case and merge to solve the bug, which is problematic in many other open PRs. |
|
@jpaillard I don't see the impact on the other PR. The test was passing before with only a warning. |
3 participants
No description provided.