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

pllim · 2024-03-15T02:31:19Z

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

Check to see if a compatibility PR is needed for stsynphot downstream.

github-actions · 2024-03-15T02:31:37Z

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.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the STScI coding guidelines?
Are tests added/updated as required? If so, do they follow the STScI testing guidelines?
Are docs added/updated as required?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions.
Did the CI pass? If no, are the failures related?
Is a change log needed?
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 · 2024-03-15T02:33:01Z

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 ⚠️

Additional details and impacted files

@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

but at a cost.

pllim · 2024-03-15T03:10:17Z

synphot/tests/test_specio.py

-    def teardown_class(self):
-        shutil.rmtree(self.outdir)
+
+def test_read_nonstandard_fits_cols_01(tmp_path):


This is the example given by @ariedel .

pllim · 2024-03-15T03:10:38Z

synphot/tests/test_specio.py

+    assert_quantity_allclose(tr(wav), trace, atol=1e-7)
+
+
+def test_read_nonstandard_fits_cols_02(tmp_path):


This is the example given by @talister .

pllim · 2024-03-15T03:12:33Z

synphot/units.py

@@ -295,6 +295,18 @@ def validate_unit(input_unit):
            output_unit = THROUGHPUT
        elif input_unit_lowcase == 'jy':
            output_unit = u.Jy
+        elif input_unit_lowcase == "flam":


If there are anymore weird units that STScI uses in any table column synphot needs to parse, we have to hardcode the exception rule here, or fix the data.

Now that we go though astropy.table.QTable, it parses all the columns. AFAIK, there is no way to skip. This is the price we pay for a more flexible table format.

simply because we have no such files in ASCII format. TST: Improve coverage, use tmp_path instead of tempfile or tmpdir

ariedel · 2024-03-19T17:23:52Z

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 · 2024-03-19T19:04:56Z

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 · 2024-03-19T19:15:30Z

p.s. I opened astropy/astropy#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.

again.

pllim · 2024-03-19T21:51:48Z

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

ariedel · 2024-03-20T21:16:52Z

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 · 2024-03-21T15:06:44Z

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

ariedel · 2024-03-21T15:15:45Z

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").

with remote data

pllim · 2024-03-21T15:50:28Z

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

pllim · 2024-03-21T15:52:40Z

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

pllim · 2024-03-21T16:38:13Z

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.

ariedel

I have now tested this, and approve.

pllim · 2024-03-22T16:23:02Z

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!

pllim added this to the 1.4.0 milestone Mar 15, 2024

pllim requested a review from ariedel March 15, 2024 02:31

github-actions bot added testing reddening specio spectrum thermal units labels Mar 15, 2024

pllim added the bug label Mar 15, 2024

BUG/FEAT: read_fits_spec now more flexible

80e7334

but at a cost.

pllim force-pushed the fits-io-table branch from e8a9607 to c685801 Compare March 15, 2024 02:52

pllim added the Build wheels Test wheels on PR label Mar 15, 2024

pllim commented Mar 15, 2024

View reviewed changes

pllim force-pushed the fits-io-table branch from 7e2fbb0 to 4a7804c Compare March 15, 2024 03:17

TST: Some lines not covered

b51e481

simply because we have no such files in ASCII format. TST: Improve coverage, use tmp_path instead of tempfile or tmpdir

pllim force-pushed the fits-io-table branch from 4a7804c to b51e481 Compare March 15, 2024 03:24

DOC: Update TUNITn in user doc.

f968204

pllim mentioned this pull request Mar 15, 2024

specio.read_spec assumes wavelength and flux are always in column 1 and column 2 #372

Closed

FITS spec I/O col lookup is case-insensitive

686bada

again.

pllim added a commit to pllim/stsynphot_refactor that referenced this pull request Mar 21, 2024

TMP: Test spacetelescope/synphot_refactor#384

8ac970c

pllim mentioned this pull request Mar 21, 2024

TMP: Test spacetelescope/synphot_refactor#384 spacetelescope/stsynphot_refactor#192

Closed

pllim added a commit to pllim/stsynphot_refactor that referenced this pull request Mar 21, 2024

TMP: Test spacetelescope/synphot_refactor#384

fa61cb1

with remote data

pllim added 2 commits March 21, 2024 11:55

Add THROUGHPUT to accepted unit string

6720c9a

Handle non-standard units more gracefully

0edcfb2

ariedel approved these changes Mar 22, 2024

View reviewed changes

pllim merged commit d2c71cf into spacetelescope:master Mar 22, 2024
23 checks passed

pllim deleted the fits-io-table branch March 22, 2024 16:06

pllim mentioned this pull request Apr 11, 2024

Update license year spacetelescope/stsynphot_refactor#193

Merged

hugobuddel mentioned this pull request Nov 17, 2024

wave_unit and flux_unit are deprecated since synphot 1.4 AstarVienna/speXtra#62

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pllim commented Mar 15, 2024 •

edited

Loading

github-actions bot commented Mar 15, 2024 •

edited by pllim

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading

pllim Mar 15, 2024

pllim Mar 15, 2024

pllim Mar 15, 2024

ariedel commented Mar 19, 2024

pllim commented Mar 19, 2024

pllim commented Mar 19, 2024

pllim commented Mar 19, 2024

ariedel commented Mar 20, 2024

pllim commented Mar 21, 2024

ariedel commented Mar 21, 2024

pllim commented Mar 21, 2024

pllim commented Mar 21, 2024

pllim commented Mar 21, 2024

ariedel left a comment

pllim commented Mar 22, 2024

		assert_quantity_allclose(tr(wav), trace, atol=1e-7)


		def test_read_nonstandard_fits_cols_02(tmp_path):

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

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

Conversation

pllim commented Mar 15, 2024 • edited Loading

Description

After merge

github-actions bot commented Mar 15, 2024 • edited by pllim Loading

codecov bot commented Mar 15, 2024 • edited Loading

Codecov Report

pllim Mar 15, 2024

Choose a reason for hiding this comment

pllim Mar 15, 2024

Choose a reason for hiding this comment

pllim Mar 15, 2024

Choose a reason for hiding this comment

ariedel commented Mar 19, 2024

pllim commented Mar 19, 2024

pllim commented Mar 19, 2024

pllim commented Mar 19, 2024

ariedel commented Mar 20, 2024

pllim commented Mar 21, 2024

ariedel commented Mar 21, 2024

pllim commented Mar 21, 2024

pllim commented Mar 21, 2024

pllim commented Mar 21, 2024

ariedel left a comment

Choose a reason for hiding this comment

pllim commented Mar 22, 2024

pllim commented Mar 15, 2024 •

edited

Loading

github-actions bot commented Mar 15, 2024 •

edited by pllim

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading