Properly handle Swift RMF when using Astropy backend (fix #357) #358
Conversation
DougBurke
changed the title
DougBurke
changed the title
|
@olaurino in fd13e89 I have to add a warning to Some options would be (off the top of my head):
Probably many others. I currently prefer the second approach, but no idea if either are possible. I have tried (in #351) to use the |
|
Just to note that we have external validation that this fixes a similar problem (this time not a SWIFT response); #357 (comment) |
sherpa/conftest.py
Outdated
| # as the test checks this warning is created it would be useful | ||
| # if the test could eitehr clear the warnings, or tell this | ||
| # fixture to ignore it, rather than making it a global flag | ||
| r"divide by zero encountered in divide", |
There was a problem hiding this comment.
I might be doing something wrong, but if I remove these lines I don't get a failure from the tests. @DougBurke could you please confirm you saw this failure before adding the warnings to the whitelist?
There was a problem hiding this comment.
I should add I tried this on Python 3, I'll try with Python 2, maybe that's the catch.
There was a problem hiding this comment.
Nope, just tried in Python 2, the tests pass even though I removed the warnings from the white list.
There was a problem hiding this comment.
@DougBurke any comments on my findings? I'd try running CI with the warnings removed from the whitelist. It bothers me that there isn't anything in the pytest docs explaining why the warnings wouldn't bubble up after you capture them with pytest itself, but then again maybe I did something wrong and Travis wouldn't be happy. This is the only (minor) issue preventing me from merging this PR.
There was a problem hiding this comment.
I managed to forget about this comment until this morning. I haven't looked at it yet (unlikely to get around to it until tonight now). I don't know if it's related that in pytest 3 they added support for reporting warnings, so I don't know if any of the things we are doing interact strangely with that code (I doubt it but just mention it). My experience with the warning code has been some fragility, in that some things don't seem reliable. This is an aside, and we should work out what's going on here before merging the code.
There was a problem hiding this comment.
I can confirm that all the tests pass on Travis after removing the warnings from the whitelist.
There was a problem hiding this comment.
I can remove these from the whitelist. I may have just added them "in case" (it's one of the reasons why I think a more fine-grained approach to marking tests that can/do raise a warning would be useful).
There was a problem hiding this comment.
I'll push the commit I already have.
There was a problem hiding this comment.
Looks good. I highlighted an apparent issue with the white list that might require some attention. Other than that, the PR could be merged.
I gave my 2 cents on some of the questions in the comments, but I am not requiring a follow up on those questions/answers.
| from sherpa.utils.err import IOErr | ||
| from sherpa.astro.data import DataARF, DataPHA, DataRMF | ||
|
|
||
| # Should each test import io instead of this? Also, do we have a |
There was a problem hiding this comment.
importing io in each test could be done with a fixture, but I am not sure what that would buy you.
| from sherpa.astro.data import DataARF, DataPHA, DataRMF | ||
|
|
||
| # Should each test import io instead of this? Also, do we have a | ||
| # better way of determining what the backend is? |
There was a problem hiding this comment.
I don't think we do. We might provide a name to the backends, but that would not be much better than what you did here. The backend "interfaces" are somewhat underspecified, so I am not sure how much sense it makes to add a name.
There was a problem hiding this comment.
The reasons for these comments are - in part - because there's the following comment in sherpa/astro/io/__init__.py
This module should not be imported directly if there is no
FITS support present (i.e. neither the crates or astropy
back ends are in use).
so I'm always a bit nervous about how best to import and use it.
|
Rebasing to get testing fixes/improvements from master. Original tip was 32478f5 |
The RMF reading is known to be broken with the AstroPy backend (issue sherpa#357).
Adds tests for invalid reads of ARF and RMF, and ensures that the expected error class is thrown when using crates of pyfits backends.
This just tests that the data can be read in as expected. It does not check it can be used in Sherpa. These responses have an energy grid starting at 0 keV which causes Sherpa problems (but that is technically outside the bug that is fixed in this PR).
The Swift data and responses are read in, and the statistic
calculated (for all data and a subset of data) for a chi-square
and Poisson-based variant. The results are compared to values
caluclated using XSPEC 12.9.1b.
The ARF and RMF have to be modified before being used, to replace
the energy-bin edge equal to 0. Asserts are used to check that the
data really contains the problem value, in case the I/O layer is
ever changed to "fudge" problem data like this (XSPEC does it when
the RMF is read in, with the message
***Warning: Detected response matrix energy bin value = 0 (or neg).
XSPEC will instead use small finite value (response file will not be altered).
The 0-value bin causes a division-by-zero warning (if it is not fixed).
This warning is checked for, but at present there doesn't seem to be
an obvious way to tell the pytext fixture in sherpa/conftest.py,
namely capture_all_warnings, that this warning should only be ignored
for this test. I have tried (in other PRs) to use the recwarn fixture
to remove the warning from the system, but it did not seem reliable
(e.g. would work if a single test file was run, but not when all tests
were run).
The only reason for this change is to try and get Travis-CI to run the tests.
Surprisingly enough this switch changed the stat value (from NaN to a finite value). This value is only used as a regression test, to ensure that we note when the behavior changes, rather than a validation that the code is doing the right thing (which is later on in this particular test).
This replaces '==' by '='. It also adds some explicit checks for the backend not being available, which should not happen but let's be carefull out there.
olaurino
force-pushed
the
bugfix-astropy-rmf-reading
branch
from
32478f5
to
6c8b45e
Compare
|
Is this okay to be marked as approved now, or are we waiting for any more code changes? |
olaurino
changed the title
PR sherpa#358 added AstroPy support for RMF files where the data was not encoded using FITS variable-length arrays for the matrix. This means that the data can not just be concatenated together. Instead each row in the matrix (which, in this case, is a regular nchannels by nbins array) has to be subset (maybe multiple times). The commit I added to do this - cfad850 worked in the Swift case, where the F_CHAN/N_CHAN values were set up to effectively remove the "space saving" mechanism they provide (that is, all elements of each row in the MATRIX column are used). It does not work for cases where F_CHAN/N_CHAN are not set to 0/nbins for each row. The code was embarassingly-confused over what it was meant to be doing, and only worked for the Swift case "by luck". This commit should fix this.
This commit adds tests and fixes the code handling the ROSAT RMF with the AstroPy I/O backend. There is a known issue with the CIAO Crates library in CIAO 4.9 which means that the ROSAT RMF can not be read in using the Crates I/O backend with Python 3.5 (it works okay with Python 2.7). The tests are therefore setup to skip certain combinations of Python and backend. The test values used as the "truth" here were calculated using CIAO 4.9/Python 2.7 and validated against XSPEC 12.9.1p. It leverages a fixture for cleaning the Sherpa ui state from changeset 0d44a3e An explanation of the fix for handling non-variable-length RMF files with the AstroPy backend: PR sherpa#358 added AstroPy support for RMF files where the data was not encoded using FITS variable-length arrays for the matrix. This means that the data can not just be concatenated together. Instead each row in the matrix (which, in this case, is a regular nchannels by nbins array) has to be subset (maybe multiple times). The commit I added to do this - cfad850 worked in the Swift case, where the F_CHAN/N_CHAN values were set up to effectively remove the "space saving" mechanism they provide (that is, all elements of each row in the MATRIX column are used). It does not work for cases where F_CHAN/N_CHAN are not set to 0/nbins for each row. The code was embarassingly-confused over what it was meant to be doing, and only worked for the Swift case because of the data's simplified structure.
This commit adds tests and fixes the code handling the ROSAT RMF with the AstroPy I/O backend. There is a known issue with the CIAO Crates library in CIAO 4.9 which means that the ROSAT RMF can not be read in using the Crates I/O backend with Python 3.5 (it works okay with Python 2.7). The tests are therefore setup to skip certain combinations of Python and backend. The test values used as the "truth" here were calculated using CIAO 4.9/Python 2.7 and validated against XSPEC 12.9.1p. It leverages a fixture for cleaning the Sherpa ui state from changeset 0d44a3e An explanation of the fix for handling non-variable-length RMF files with the AstroPy backend: PR sherpa#358 added AstroPy support for RMF files where the data was not encoded using FITS variable-length arrays for the matrix. This means that the data can not just be concatenated together. Instead each row in the matrix (which, in this case, is a regular nchannels by nbins array) has to be subset (maybe multiple times). The commit I added to do this - cfad850 worked in the Swift case, where the F_CHAN/N_CHAN values were set up to effectively remove the "space saving" mechanism they provide (that is, all elements of each row in the MATRIX column are used). It does not work for cases where F_CHAN/N_CHAN are not set to 0/nbins for each row. The code was embarassingly-confused over what it was meant to be doing, and only worked for the Swift case because of the data's simplified structure.
This commit adds tests and fixes the code handling the ROSAT RMF with the AstroPy I/O backend. There is a known issue with the CIAO Crates library in CIAO 4.9 which means that the ROSAT RMF can not be read in using the Crates I/O backend with Python 3.5 (it works okay with Python 2.7). The tests are therefore setup to skip certain combinations of Python and backend. The test values used as the "truth" here were calculated using CIAO 4.9/Python 2.7 and validated against XSPEC 12.9.1p. It leverages a fixture for cleaning the Sherpa ui state from changeset 0d44a3e An explanation of the fix for handling non-variable-length RMF files with the AstroPy backend: PR sherpa#358 added AstroPy support for RMF files where the data was not encoded using FITS variable-length arrays for the matrix. This means that the data can not just be concatenated together. Instead each row in the matrix (which, in this case, is a regular nchannels by nbins array) has to be subset (maybe multiple times). The commit I added to do this - cfad850 worked in the Swift case, where the F_CHAN/N_CHAN values were set up to effectively remove the "space saving" mechanism they provide (that is, all elements of each row in the MATRIX column are used). It does not work for cases where F_CHAN/N_CHAN are not set to 0/nbins for each row. The code was embarassingly-confused over what it was meant to be doing, and only worked for the Swift case because of the data's simplified structure.
This commit adds tests and fixes the code handling the ROSAT RMF with the AstroPy I/O backend. There is a known issue with the CIAO Crates library in CIAO 4.9 which means that the ROSAT RMF can not be read in using the Crates I/O backend with Python 3.5 (it works okay with Python 2.7). The tests are therefore setup to skip certain combinations of Python and backend. The test values used as the "truth" here were calculated using CIAO 4.9/Python 2.7 and validated against XSPEC 12.9.1p. It leverages a fixture for cleaning the Sherpa ui state from changeset 0d44a3e An explanation of the fix for handling non-variable-length RMF files with the AstroPy backend: PR sherpa#358 added AstroPy support for RMF files where the data was not encoded using FITS variable-length arrays for the matrix. This means that the data can not just be concatenated together. Instead each row in the matrix (which, in this case, is a regular nchannels by nbins array) has to be subset (maybe multiple times). The commit I added to do this - cfad850 worked in the Swift case, where the F_CHAN/N_CHAN values were set up to effectively remove the "space saving" mechanism they provide (that is, all elements of each row in the MATRIX column are used). It does not work for cases where F_CHAN/N_CHAN are not set to 0/nbins for each row. The code was embarassingly-confused over what it was meant to be doing, and only worked for the Swift case because of the data's simplified structure.
Release Note
A Swift RMF could not be read in when the AstroPy back end was in use. The problem was that the code did not support RMF matrices that were not stored as variable-length arrays. This has now been fixed, and new tests have been added for this kind of files.
Details
Fixes issue #357, which meant that a Swift RMF could not be read in when the AstroPy back end was in use. The problem was that the code did not support RMF matrices that were not stored as variable-length arrays. The PR adds tests of reading in Swift PHA, ARF, and RMF files, as well as checks that you can not read in a RMF as a PHA (and various other options), and a simple check that the files work with the
sherpa.astro.uiAPI.The test that all the files work together calculates the statistic for a model over a fixed energy range, comparing the values to those calculated manually using XSPEC. To do this the code changes the RMF and ARF, to replace the 0-energy lower-edge of the first bin by a small positive value.
I would like to remove pr:hold, but this needs sherpa/sherpa-test-data#11 to be accepted first. I also don't know why the travis tests haven't been run for any of these commits (perhaps because I didn't handle the submodule update correctly?).