Fix err dist none

[andresgur]

Fixes #908

Provide an overview of the implemented solution or the fix and elaborate on the modifications.

I have modified the valid_statistics in lightcurve.py. Instead of the using the old None, now we use "None".

valid_statistics = ["poisson", "gauss", "None"]

I have also changed the default err_dist in the Lightcurve constructor to "None" to correctly reflect the docs.

from stingray import Lightcurve
import numpy as np
from stingray.lightcurve import valid_statistics


times = np.arange(1000)

mean_flux = 100.0
std_flux = 2.0 
flux = np.random.normal(loc=mean_flux, scale=std_flux, size=len(times))
flux_err = np.ones_like(flux) * std_flux

print(valid_statistics)

lc = Lightcurve(times, flux, err=flux_err, err_dist="None", dt=1.0, skip_checks=True)
np.testing.assert_array_equal(lc.counts_err, np.full(lc.n, std_flux))


lc = Lightcurve(times, flux, err=None, err_dist="None", dt=1.0, skip_checks=True)
np.testing.assert_array_equal(lc.counts_err, np.zeros(lc.n))

[andresgur]

we do .lower() so either "None" or "none" is fine from the user perspective. I kept "None" as that's what e.g. matplolib uses. But we could keep it as "none" in valid_statistics as poiss and gauss are not capitalized, but from the user perspective it does not matter.

Having None type would make the code more clunky and we would unnecessarily need to handle that case separately, while if "none" is taken as a string makes all the cases the same and the code more straightforward.

[matteobachetti]

@andresgur Thanks for your PR. I made a slight modification to avoid breaking the API.

[andresgur]

thanks @matteobachetti ! Is there anything else for me to do? Sorry still learning this whole PR thing

[matteobachetti]

thanks @matteobachetti ! Is there anything else for me to do? Sorry still learning this whole PR thing

No worries! I'm taking care of solving the API change issues. Please write a changelog snippet according to https://docs.stingray.science/en/stable/contributing.html#updating-and-maintaining-the-changelog. It should be called docs/changes/909.bugfix.rst and briefly describe the changes

[ankitkhushwaha]

Hey @andresgur , i didn't see your work here and fixes err_dist issue in #912 .
But i think it is good to support the None object not the str one. Users always more tends to use None (object) not "none".

[matteobachetti]

@andresgur the change of default means that a warning is thrown everywhere in the code now during tests, making about 50 tests fail.
I think you found an important inconsistency in the docs, and defaulting to "none" is indeed more correct, but this also means that we need to change a lot of tests, catching the warning with constructs like

with pytest.warns(UserWarning, match="Beware! Stingray only uses"):
     <etc.>

[andresgur]

Added the file @matteobachetti

[ankitkhushwaha]

the change of default means that a warning is thrown everywhere in the code now during tests, making about 50 tests fail. I think you found an important inconsistency in the docs, and defaulting to "none" is indeed more correct, but this also means that we need to change a lot of tests, catching the warning with constructs like

with pytest.warns(UserWarning, match="Beware! Stingray only uses"):
     <etc.>

Hey @andresgur, we also need to fix the tests raising above warning.

[andresgur]

which ones? No tests break for me... I thought the problem was keeping none as a string

[ankitkhushwaha]

which ones? No tests break for me... I thought the problem was keeping none as a string

did u run pytest in your machine?
Api is returning new warning, check once.

[andresgur]

for file in test*.py ; do echo $file; python $file -m unitest; done

test_base.py
test_bexvar.py
test_bispectrum.py
test_covariancespectrum.py
test_crosscorrelation.py
test_crossspectrum.py
test_events.py
Traceback (most recent call last):
File "/home/andresgur/scripts/pythonscripts/stingray/stingray/tests/test_events.py", line 8, in
from ..events import EventList
ImportError: attempted relative import with no known parent package
test_filters.py
Traceback (most recent call last):
File "/home/andresgur/scripts/pythonscripts/stingray/stingray/tests/test_filters.py", line 6, in
from ..filters import Window1D, Optimal1D, filter_for_deadtime
ImportError: attempted relative import with no known parent package
test_fourier.py
test_gti.py
test_io.py
Traceback (most recent call last):
File "/home/andresgur/scripts/pythonscripts/stingray/stingray/tests/test_io.py", line 8, in
from ..io import split_numbers
ImportError: attempted relative import with no known parent package
test_lightcurve.py
test_lombscargle.py
test_multitaper.py
test_power_colors.py
test_powerspectrum.py
test_sampledata.py
Traceback (most recent call last):
File "/home/andresgur/scripts/pythonscripts/stingray/stingray/tests/test_sampledata.py", line 1, in
from ..sampledata import sample_data
ImportError: attempted relative import with no known parent package
test_spectroscopy.py
test_stats.py
test_utils.py
test_varenergyspectrum.py

Am I missing something?

[ankitkhushwaha]

Hey @andresgur, for stingray we uses pytest for testing

inside the stingray repository run
pytest
it will show the test results

u may refer this https://docs.stingray.science/en/stable/#test-suite

[andresgur]

thanks I see it now... so we should skip the warnings as @matteobachetti suggested?

[ankitkhushwaha]

thanks I see it now... so we should skip the warnings as @matteobachetti suggested?

yes,
You need to replace the text "simon ..." with "Beware! Stingray only uses".
same as suggested by @ matteobachetti

[matteobachetti]

@andresgur tests are run using pytest or, better, tox -e test.
In any of the failed test suites above, you will find about 50 tests that failed.
We should look at each of them and, if the Poisson stats was an adequate default (the light curve is being formed by integer numbers of counts), we should add err_dist="poisson" to the Lightcurve initialization.
Otherwise, the line that throws the warning should be put inside the construct I made above.

[andresgur]

All tests passing on my end now

[ankitkhushwaha]

Hey @andresgur , rest of changes are fine, but
users when trying this example importing pytest doesn't seem to good way.

  >>> with pytest.warns(UserWarning, match="Beware! Stingray only supports poisson err_dist at the moment in many methods, and 'gauss' in a few more."):
  ...    lc = sim.simulate(2)

instead this u can just suppress the warning

>>> lc = sim.simulate(2) # doctest: +IGNORE_WARNINGS

once this pr gets merged, i will write tests for #912

[dhuppenkothen]

Regarding this warning, I’m wondering if we could document which methods only support Poisson, which support Gauss and put that into the docs, with a link to that page in the warning? I feel like if I were the user, I’d like to know which methods I could use with which distribution. :)

I’m assuming this was probably in there before this PR, so we can address this elsewhere, but it seemed worth raising.

[ankitkhushwaha]

probably worth of raising an issue.
see #916

[andresgur]

@ankitkhushwaha I made the requested change btw, and yes @dhuppenkothen that would be a very good idea to add detail of the methods that handle Gauss/Poiss errors

[ankitkhushwaha]

Hey @andresgur, maintainers are quite busy around this time, so it might take a while for the PR to get reviewed.
Hope u understand.

@Sauravroy34 has mentioned some places where docs related to distribution error can be imporved.