XraySpectrum1D class #178
XraySpectrum1D class #178
Conversation
…y_resp is in correct order. No longer need to check
|
Please review this addition. A note about tests: I have tested Chandra HETG files and fitting on my own computer. This requires downloading external files, and using an optimizer (e.g. Testing ARF and RMF objects will also require downloading external files. |
|
A first pass of some comments, but will have to look at some more. Depending on the size of the files, we should use the astropy remote data for the testing. Having scipy as an optionally dependency is okay. If it is for tests, that can be added so a test is skipped when scipy is missing. I can look an example if you need one |
There was a problem hiding this comment.
I'm a little queasy about class variables being defined outside the __init__ method (e.g. e_low, e_high, etc). It'd be great if they could be initialized beforehand somewhere to avoid possible attribute errors. Thoughts, @stscicrawford?
Also, there are some extraneous pass and returns floating about that would be great to see removed just for the sake of readability.
|
Hey! This branch has been updated to the latest specutils/master and most of the tests are passing (aside from some docstring indentation issues). @nmearl @hamogu @stscicrawford Can one of you do the final review? Current Travis error: Somewhat related? |
|
@eblur I appreciate the trust you put in me, put I don't have the authority to do final reviews in specutils. |
There was a problem hiding this comment.
@eblur You asked for it, so here it is... Note that I reviewed how I would do in astropy, which is a far more mature package. Some of my suggestions can probably just be converted into issues or ignored if you don't feel like it in a fast moving package like specutils where thinks are still likely to change when the code starts to get used on real data. I would also include a link to the OGIP memo that defines the file formats for arfs and rmfs (https://heasarc.gsfc.nasa.gov/docs/heasarc/caldb/docs/memos/cal_gen_92_002/cal_gen_92_002.html). While X-ray astronomers might be familiar with the machinery, even we don't have the definitions of n_grp etc. memorized. For all other people, understanding, maintaining or improving this code is impossible without a link to the corresponding standard definition.
| model_flux = new_model(test_spec.spectral_axis.value) | ||
| ymodel = test_spec.apply_response(model_flux) | ||
| assert len(ymodel) == len(test_spec.spectral_axis) | ||
| return |
There was a problem hiding this comment.
no empty return needed. Unless that's the spectutils style - some specutils maintainer will have to tell us how they want it.
| new_model = PowerLaw1D(amplitude=5.e-3, alpha=1.8, x_0=1.e3) | ||
| model_flux = new_model(test_spec.spectral_axis.value) | ||
| ymodel = test_spec.apply_response(model_flux) | ||
| assert len(ymodel) == len(test_spec.spectral_axis) |
There was a problem hiding this comment.
I would feel better if this not only tested that the calculation works, but also that the numbers are right. Can you add a
ymodel[234] = 234 just for one or two bins? Either get the number from a known good program (ISIS is clearly absolutely 100% bug free people in my corridor tell me) or at the very least from a case you looked at by hand that looks good so that we notice if some other change affects these numbers.
specutils/spectra/xrayspectrum1d.py
Outdated
| bin_hi : numpy.ndarray | ||
| The right edges for bin values | ||
|
|
||
| bin_unit : string OR astropy.units.Unit |
There was a problem hiding this comment.
Should this be
`astropy.units.Unit`
so that sphinx build the links to the astropy docs?
There was a problem hiding this comment.
Same is true for all later docs, in particular for those that relate to other classes in the same file, e.g arf or rmf.
| if self.arf is not None: | ||
| mrate = self.arf.apply_arf(mflux, exposure=exposure) | ||
| else: | ||
| #print("Caution: no ARF file specified") |
| file (RMF), apply both. Otherwise, apply whatever response is in RMF. | ||
|
|
||
| The model flux spectrum *must* be created using the same units and | ||
| bins as in the ARF (where the ARF exists)! |
There was a problem hiding this comment.
I wonder if there is a way to enforce that somehow. Seems like a classic point of failure.
| # get the corresponding value | ||
| tlmin = np.int(list(hdr.items())[tlmin_idx][1]) | ||
|
|
||
| return tlmin |
There was a problem hiding this comment.
I'm confused. Can't you get the same output as this function simply with return int(h.header['TLMIN'])?
|
|
||
| # stack all nonzero rows in n_chan and f_chan | ||
| #n_chan_flat = np.hstack(n_chan[nz_idx]) | ||
| #f_chan_flat = np.hstack(f_chan[nz_idx]) |
There was a problem hiding this comment.
I think this should either be used (if it should be used) or deleted.
|
|
||
| All of this is basically a big bookkeeping exercise in making | ||
| sure to get the indices right. | ||
|
|
There was a problem hiding this comment.
In fact, it's so painful as a bookkeeping exercise that I won't do it by hand now. That's why it's so important to test with at least one real-world rmf in the tests below.
|
Replaced this PR with #427 |
First pass at XraySpectrum1D class.
Currently working on example cases to test that the spectrum and response files play nicely.