BUG/API: read_fits_spec now more flexible but at a cost

[pllim]

Description

This pull request is to address the problem of read_fits_spec unnecessarily hardcoding TUNIT1 and TUNIT2. However, in making it more flexible, some breaking changes are unavoidable. See change log added here.

Fixes #372

After merge

• [x] Check to see if a compatibility PR is needed for stsynphot downstream.

[github-actions[bot]]

Thank you for your contribution! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• [x] Do the proposed changes actually accomplish desired goals?
• [x] Do the proposed changes follow the STScI coding guidelines?
• [x] Are tests added/updated as required? If so, do they follow the STScI testing guidelines?
• [x] Are docs added/updated as required?
• [x] Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions.
• [x] Did the CI pass? If no, are the failures related?
• [x] Is a change log needed?
• [x] Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 98.24561% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 96.83%. Comparing base (2c1f2a5) to head (0edcfb2). Report is 14 commits behind head on master.

Files	Patch %	Lines
synphot/specio.py	96.55%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #384 +/- ## ========================================== + Coverage 96.80% 96.83% +0.03% ========================================== Files 17 17 Lines 2034 2056 +22 ========================================== + Hits 1969 1991 +22 Misses 65 65 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[ariedel]

I have tried this branch, and I am concerned about the removal of the case insensitivity. The ETC web interface uses synphot to read in user-uploaded spectra; I worry that this would lead to rejection of a large number of spectra and a worse user experience. (This branch has also revealed that the ETC's own data files are very heterogeneous when it comes to capitalization (@rizeladiaz).)

Part of the reason the original issue appeared is that the ETC is now trying to expand our use of Synphot to reading in files that aren't actually spectra or spectral elements (quantum yield curves, dispersion files) in a unit-aware way. If you have an hour or half hour of time to chat with us about the best ways to accomplish this, it would be much appreciated.

[pllim]

case insensitivity

This is a side effect of using Unified I/O in astropy. All the parsing is done now in https://github.com/astropy/astropy/blob/main/astropy/io/fits/connect.py . I was surprised too that it is case-sensitive, so maybe this is a bug that should be fixed upstream. But I will have to ask my astropy colleagues.

reading in files that aren't actually spectra or spectral elements (quantum yield curves, dispersion files) in a unit-aware way

If you have no plans to use these objects within synphot for further analysis, you might be abusing the parser here and better off using something more generic like astropy.

[pllim]

p.s. I opened https://github.com/astropy/astropy/issues/16221 but now that I have thought about it as I typed it up, maybe I can force the case-insensitivity here. Will report back.

[pllim]

@ariedel , I managed to make it case insensitive again. Is this good enough for you?

[ariedel]

This is much better. It's potentially revealed more problems in STScI's TRDS dataset, but I don't think Synphot should be responsible for them @rizeladiaz

• I can't load the Bruzual77 spectra from $PYSYN_CDBS/grid/bz77; all of them yield ValueError: Function units cannot be written in fits format. Only generic, unscaled, latex, latex_inline, unicode, console are supported. I have not taken the time to track down what that error means yet or how it could be worked around.
• I also can't normalize anything by the 2MASS H bandpass in $PYSYN_CDBS/comp/nonhst/2mass_h_001_syn.fits because it has a unit of "THROUGHPUT" which isn't recognized by Astropy or Synphot. (I'll need to specially define it as equivalent to u.dimensionless_unscaled in our normalization code.)

[pllim]

Ah, yes, I didn't test this in stsynphot. Maybe I should.

[ariedel]

I'm pretty sure it's because the flux unit in those bz77 files is "STMAG". The ETC is trying to use syn.spectrum.SourceSpectrum.from_file(filepath) to read it, rather than any explicit STSynphot function; the error is coming from line 201: subhdu.header[key] = newval.to_string("fits").

[pllim]

If it is just a matter of weird unit strings, we can add exception rules here if they are not too weird.

[pllim]

p.s. Did not seem to trip up stsynphot, at least for the stuff that I test for.

[pllim]

Actually, I take it back. I found bz77 at https://stsynphot.readthedocs.io/en/latest/stsynphot/appendixa.html#stsynphot-appendixa-bz77 and indeed the example broke with this patch. I pushed a couple of commits to try to make the code more flexible. Please try again with the updated PR branch and let me know if I missed anything else. Theoretically, the parser here should be compatible with things we advertise.

[pllim]

FWIW I restarted the devdeps job at https://github.com/spacetelescope/stsynphot_refactor/actions/runs/8338326996 and it still passed. But if you find anything amiss, please open new issue(s). Thanks for the review!