Warning cleanup and cleanups of spectral fitting notebooks

[jdbuhler]

This patch seeks to remove unnecessary warnings from the test suite and at least some of the notebooks. There are a couple of small changes (modulo whitespace noise) to fix some test cases:

(1) adding HIERARCH to a FITS tag that is too long for a standard card;
(2) preventing floating-point overflow by limiting the parameter range in one test case of background fitting;
(3) turning off unnecessary printing in a COSILike test case to avoid two floating-point warnings from threeML that do not impact the result of optimization.

The remainder of the patch is a cleanup of the spectral fitting notebooks, which completes work that was partially done the recent integration patch. Some threeML warnings were suppressed by adding sensible bounds to some parameters. The dependencies were cleaned up, and some code (mostly related to plotting) was simplified. The graphs produced by these notebooks look the same as before.

There is one change for which I'd like an expert review. In Example 2 of the diffuse 511 spectral fit notebook, the prior code provided the location of the point source (src1) to astromodels' PointSource class in ICRS coordinates, even though the natural specification was galactic l=0, b=0. Contrary to the comment in that code block, PointSource does in fact accept galactic coordinates. However, setting them in galactic form changes the result of the optimization -- the point source's intensity no longer goes to 0. I get the sense that this problem is degenerate, so the change is not actually an error, but someone familiar with this particular optimization task should take a look.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.51%. Comparing base (99bed81) to head (d0f4833).

Files with missing lines	Coverage Δ
cosipy/event_selection/good_time_interval.py	92.23% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[israelmcmc]

@jdbuhler: I checked your last point (relevant diff in screenshot, for convenience).

I think it's pointing to a bug, the fit result shouldn't change that much just from a coordinate change. I quickly tested astromodels, but it's working as expected:

from astromodels import PointSource, Gaussian
import astropy.units as u
from astropy.coordinates import SkyCoord

c = SkyCoord(l=0*u.deg, b=0*u.deg, frame='galactic')
c_icrs = c.transform_to('icrs')

spectrum2 = Gaussian()

# Define source:
src2 = PointSource('point_source', ra = c_icrs.ra.deg, dec = c_icrs.dec.deg, spectral_shape=spectrum2)
src2g = PointSource('point_source', l = c.l.deg, b = c.b.deg, spectral_shape=spectrum2)

print(src2g.position.sky_coord.separation(src2.position.sky_coord)) #-> 0d00m00s

I guess it's a problem in our side, but I haven't been able to find it (I didn't have much time today).

I'm tagging @ckarwin since he added that comment about astromodels. Maybe he remembers seeing this problem before.

[jdbuhler]

The coordinate change is the responsible one - if I revert to passing ra/dec transformed from l=0,b=0, the results are as in the develop branch.

[israelmcmc]

Right, I just think you've uncovered a bug that as present before your changes. Whether you use galactic or icrs the results should be the same within numerical precision, because they are both really the same direction.

[jdbuhler]

OK, I did some analysis of what is going on, and I think there is a coordinate system mismatch issue.

If a PointSource object 'source' is initialized with ra/dec arguments, then source.position.sky_coord will return an ICRS-frame SkyCoord. That coordinate in turn is passed to the response's get_point_source_response() method, which computes the PSR in the coordinate frame of the source (i.e., ICRS). But then we use that PSR to compute an expectation which is compared to the observed data -- which in this case is in the galactic frame, not ICRS.

If we instead initialize source with l/b arguments, the resulting SkyCoord, and hence the PSR, will be in the galactic frame, which I think would be the correct behavior here. The resulting fit would then be the one I'm seeing after my change, not the one seen in develop that sends the point source's intensity to 0.

More generally, if the data is not local-frame, we should transform the source SkyCoord into the frame of the data before computing the PSR. If it is local-frame, the SpacecraftFile's get_target_in_sc_frame() should take care of any transformation needed to produce a correct local-frame path. [But I'm not sure that is done right either -- get_target_in_sc_frame() appears to assume that the source is in the same frame as the SpacecraftFile's attitude array, which is pretty much always galactic, rather than forcing a transformation if the source happens to be ICRS.]

Does this sound right?

[jdbuhler]

I will add that this issue may affect other parts of cosipy as well. For instance, I think the source injector is supposed to return galactic-frame expectations (since one of the options is to provide a galactic-frame response), but if the source is in any other inertial frame, the returned PSR will be in the source's frame, not the galactic frame. [I believe that for a galactic-frame response, the ang2pix call at the beginning of get_point_source_response() transforms the source to the NuLambda frame of the response, i.e., to galactic, before computing the pixel value.]

Polarization ASAD analysis always transforms the source to ICRS when an inertial-frame analysis is requested, which I assume is the right thing. I am not sure what is supposed to happen if the requested frame is a spacecraftframe.

In ContinuumEstimation, calc_psr() returns the PSR in the frame of the source, but the docstring says it should always be in galactic frame.

ts_map with a local-frame response converts galactic-frame source directions to 3-vectors before passing them off to get_target_in_sc_frame(), so it is responsible for first checking that the orientation history is galactic-frame. It doesn't do so right now -- we should probably use the frame of the SpacecraftFile when generating the set of source vectors to investigate. With a galactic-frame response, it does the right thing (source vectors are all galactic-frame).

There may be more, but these are the ones that I noticed.