MNT: Port from pysynphot to stsynphot #388

pllim · 2021-02-04T22:48:44Z

Fix #362

cc @mperrin

TODO

Have stakeholders review and test this.
Fix code and tests, if necessary.

pllim

Someone needs to test this manually as I don't think your CI currently run the tests that requires PySynphot.

poppy/instrument.py

pllim · 2021-02-04T23:32:32Z

poppy/instrument.py


            wave_bin_edges = np.linspace(minwave, maxwave, nlambda + 1)
            wavesteps = (wave_bin_edges[:-1] + wave_bin_edges[1:]) / 2
            deltawave = wave_bin_edges[1] - wave_bin_edges[0]
+            jwst_area = 25.4 * (units.m * units.m)


stsynphot no longer let you set-and-forget the area like PySynphot does, because I deemed the risk of forgetting and then silently using the wrong value to be not worth the convenience.

pllim · 2021-02-04T23:35:26Z

poppy/utils.py

        elif sptype == "Flat spectrum in F_lambda":
-            spec = pysynphot.FlatSpectrum(1, fluxunits='flam')
-        spec.convert('flam')
-        return spec * (1. / spec.flux.mean())


I don't know why it is being normalized here. In the refactored package, flat spectrum is analytic and wavelength to evaluate it over isn't known ahead of time, so it is quite hard to normalize like this.

pllim · 2021-02-04T23:36:09Z

poppy/utils.py

        if ans is None:
            raise ValueError("Invalid power law specification cannot be parsed to get exponent")
        exponent = float(ans.groups(0)[0])
-        # note that Pysynphot's PowerLaw class implements a power law in terms of lambda, not nu.
+        # note that synphot's PowerLaw class implements a power law in terms of lambda, not nu.


It is now possible to define powerlaw in frequency space but that functionality isn't widely tested.

codecov · 2021-02-05T00:23:53Z

Codecov Report

Merging #388 (0692a72) into develop (f3feb10) will increase coverage by 2.40%.
The diff coverage is 52.63%.

❗ Current head 0692a72 differs from pull request most recent head aef3cb2. Consider uploading reports for the commit aef3cb2 to get more accurate results

@@             Coverage Diff             @@
##           develop     #388      +/-   ##
===========================================
+ Coverage    71.44%   73.85%   +2.40%     
===========================================
  Files           18       17       -1     
  Lines         6066     6051      -15     
===========================================
+ Hits          4334     4469     +135     
+ Misses        1732     1582     -150

Impacted Files	Coverage Δ
poppy/matrixDFT.py	`98.29% <ø> (+39.77%)`	⬆️
poppy/utils.py	`52.37% <0.00%> (-0.54%)`	⬇️
poppy/instrument.py	`67.28% <64.44%> (+10.00%)`	⬆️
poppy/__init__.py	`83.87% <100.00%> (-0.51%)`	⬇️
poppy/active_optics.py
poppy/dms.py	`46.13% <0.00%> (+0.55%)`	⬆️
poppy/optics.py	`81.82% <0.00%> (+0.56%)`	⬆️
poppy/poppy_core.py	`80.12% <0.00%> (+0.56%)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3feb10...aef3cb2. Read the comment docs.

MNT: Move conftest.py to root level for tox to pick up test header. MNT: Modernize test setup a little. TST: Use tmpdir fixture to avoid generating untracked files. TST: Add stsynphot to coverage run.

pllim · 2021-02-05T16:13:15Z

Update: I got the tests to pass locally, but I don't know how good is your test coverage, so please review this PR carefully.

p.s. Would be nice if your package uses Quantity throughout; it defeats the purpose of using Quantity to only manually convert the units back and forth; but that is out of scope of this PR.

pllim · 2021-02-05T16:15:08Z

Unrelated to this PR but I see this warning on your test page: Please be aware travis-ci.org will be shutting down in several weeks, with all accounts migrating to travis-ci.com. Please stay tuned here for more information.

So, you either need to move to their .com service and start paying, or port your testing infrastructure to GitHub Actions. I see that you already use tox, so doing the latter should be very easy for you.

mperrin · 2021-02-05T17:20:30Z

Thanks @pllim! @shanosborne can you give this a look, please.

@pllim: on Travis yes we're aware, and @shanosborne has been coordinating on that with others (ideally this should be solved consistently for many/most of the INS packages, not on a case-by-case basis)

About quantities, agreed in principle. However, the quantity framework becomes somewhat complicated to use with Fourier transforms (which is the core of what this package does...), and further there were noticeable performance impacts if I tried to use quantity throughout in one test PR. Admittedly that was a while ago (a couple years). It's possible some of this could be improved, but I agree it's out of scope for this PR

pllim · 2021-02-05T17:22:40Z

Re: Performance of Quantity -- Might worth checking out https://docs.astropy.org/en/latest/units/index.html#performance-tips and see if it is of any use to you.

shanosborne

This PR looks good, thanks @pllim! We can't merge it right now because we've determined that the current dev version of WebbPSF can't run with PR and needs some updates (basically the same pysynphot->stsynphot updates). So we'll merge this PR once we have a comparable PR in WebbPSF ready to go.

pllim · 2021-02-22T18:31:10Z

@shanosborne , not sure what you mean, as I developed this PR against your default branch and it is not showing that there is a conflict, but however you want to do this is fine by me. I do recommend that you test the changes extensively downstream before accepting the changes. Thanks!

shanosborne · 2021-02-22T20:27:33Z

@pllim You were able to run the development version of webbpsf with the version of poppy in this PR? I agree that this version of poppy runs fine on it's own, but the downstream webbpsf package fails for me with this poppy change with the error of AttributeError: 'ArraySpectralElement' object has no attribute 'waveset'.

pllim · 2021-02-22T20:29:56Z

Ah, sorry, I missed the fact that WebbPSF is a separate repository!

Hard to tell without the full traceback. What would be useful is if you tell me where the corresponding WebbPSF PR is and I can look at its CI log. Thank you!

mperrin

This looks good. @shanosborne I have a few (minor, optional) suggestions below.

One general question: has anyone done yet any performance benchmarking? I'm not expecting major speed changes either way, I just want to verify at a basic level there aren't any surprises.

Side topic, I see this includes some better usage of temp directories in some of the unit tests - thanks, that's needed to be done for a while.

mperrin · 2021-03-29T15:02:51Z

conftest.py

@@ -0,0 +1,26 @@
+# this contains imports plugins that configure py.test for astropy tests.


What's the story with this added contest.py file? This is pretty cryptic in parts, and I'd like to understand whether this is needed for some of the tests of synphot, whether it provides other benefits, and/or whether this causes any other changes in the behavior of the test system.

I'd like to request another sentence or two of comments here to clarify what this stuff is for, and/or simplifying if any of this is not strictly needed.

And also, is the package root directory really the place this file should be? I think in that case it does not get installed into site-packages since it's not within the poppy package subdirectory, right? Is that the intended behavior?

Story: The way tox is being run, pytest is not seeing (some of?) your poppy/conftest.py, so the test header isn't being displayed correctly in the CI logs (go see for yourself). This fixes the header.

Also see astropy/pytest-astropy-header#15 (comment) .

As for how you want to document this story in a way that you understand, feel free to use the "suggestion" feature. I have been doing this for so long that I might not document it the way that make sense to you.

mperrin · 2021-03-29T15:09:12Z

poppy/instrument.py


            wave_bin_edges = np.linspace(minwave, maxwave, nlambda + 1)
            wavesteps = (wave_bin_edges[:-1] + wave_bin_edges[1:]) / 2
            deltawave = wave_bin_edges[1] - wave_bin_edges[0]
+            jwst_area = 25.4 * (units.m * units.m)


We can simply delete this line with the jwst_area and remove the usage of it below. poppy is not interested in absolute fluxes, just normalized PSFs, so we can simplify and avoid hard-coding in this parameter with no change in results.

So area is a required parameter for synphot.Observation, but we can change it so it just takes in 1 m^2 rather than a specific number related to JWST.

area is required but it is up to you what value to give it.

In PySynphot, it was also required but PySynphot just automatically used HST area unless you overwrite the setting.

mperrin · 2021-03-29T15:12:02Z

poppy/instrument.py

            filterfile = self._filters[self.filter].filename
-            filterfits = fits.open(filterfile)
+            filterfits = fits.open(filterfile)  # TODO: Use context manager


This #TODO could be done now simply by replacing this line and the next with filterdata = fits.getdata(filterfile), I think.

mperrin · 2021-03-29T15:20:08Z

Oh and I see we now have some conflicts that need to be resolved before this is mergeable.

pllim

It is my understanding that @shanosborne has taken over the PR, so I'll let her handle the code pushes to address the reviews. I will be available for consultation. Please let me know if you have further concerns or questions. Thanks!

pllim · 2021-03-29T15:27:43Z

conftest.py

@@ -0,0 +1,26 @@
+# this contains imports plugins that configure py.test for astropy tests.


Story: The way tox is being run, pytest is not seeing (some of?) your poppy/conftest.py, so the test header isn't being displayed correctly in the CI logs (go see for yourself). This fixes the header.

Also see astropy/pytest-astropy-header#15 (comment) .

As for how you want to document this story in a way that you understand, feel free to use the "suggestion" feature. I have been doing this for so long that I might not document it the way that make sense to you.

…stsyn-port

…stsyn-port # Conflicts: # poppy/instrument.py # tox.ini

shanosborne · 2021-03-29T19:40:45Z

@mperrin I've done the updates you requested and fixed the merge conflicts. I'll do some bench-marking next, but will likely not get it done today, just a heads up!

shanosborne · 2021-03-30T20:10:23Z

@mperrin Quick bench marking results. I ran the following default calculation several times, both with the code in this PR and the code in the current master branch (confirming that stsynphot and pysynphot were called respectively):

import poppy
inst = poppy.Instrument()
%timeit inst.calc_psf()

My average results:
STSynphot Case: 3.08 s ± 103 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
Pysynphot Case: 2.86 s ± 51 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

mperrin · 2021-04-01T01:29:15Z

@shanosborne Thanks. Looks like stsynphot is perhaps a little slower than pysynphot (for whatever reason), but in any case not drastically different. Those two measurements are consistent at ~1 sigma in any case. @pllim that sound right to you?

I just wanted to check this to verify no surprises. We want to move to stsynphot in any case so I will go ahead and merge this

pllim · 2021-04-01T14:31:31Z

Looks like stsynphot is perhaps a little slower than pysynphot

I agree with @mperrin that they are consistent within uncertainty. But if you really want to drill down on it, you can do a more detailed profiling and see where the bulk of the processing time is in both. stsynphot does include overhead of astropy units and models, but it also does not sample until needed, so it really depends on how you are using it.

Initial port from pysynphot to stsynphot

454310c

pllim mentioned this pull request Feb 4, 2021

Migrate from pysynphot to synphot #362

Closed

pllim commented Feb 4, 2021

View reviewed changes

Revert unwanted change

3f1298e

pllim added 2 commits February 5, 2021 11:08

TST: Fix tests based on local run.

001418a

MNT: Move conftest.py to root level for tox to pick up test header. MNT: Modernize test setup a little. TST: Use tmpdir fixture to avoid generating untracked files. TST: Add stsynphot to coverage run.

Fix typo in conftest.py

e2465d7

Fix CI matrix setup for tox

9cff85f

mperrin requested a review from shanosborne February 5, 2021 17:39

shanosborne reviewed Feb 22, 2021

View reviewed changes

shanosborne mentioned this pull request Mar 1, 2021

Adding synphot spacetelescope/webbpsf#424

Merged

shanosborne added 2 commits March 9, 2021 07:48

Add fix for increasing wavelength range of source

f02c3c9

Merge branch 'develop' into pysyn-stsyn-port

9fcda94

shanosborne marked this pull request as ready for review March 29, 2021 14:52

shanosborne requested a review from mperrin March 29, 2021 14:52

mperrin approved these changes Mar 29, 2021

View reviewed changes

pllim commented Mar 29, 2021

View reviewed changes

shanosborne added 3 commits March 29, 2021 11:59

review updates

1aaf170

Merge branch 'pysyn-stsyn-port' of github.com:pllim/poppy into pysyn-…

59d3a2a

…stsyn-port

Merge branch 'develop' of github.com:spacetelescope/poppy into pysyn-…

0692a72

…stsyn-port # Conflicts: # poppy/instrument.py # tox.ini

Merge branch 'develop' into pysyn-stsyn-port

aef3cb2

mperrin merged commit 84b8ceb into spacetelescope:develop Apr 1, 2021

pllim deleted the pysyn-stsyn-port branch April 1, 2021 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNT: Port from pysynphot to stsynphot #388

MNT: Port from pysynphot to stsynphot #388

pllim commented Feb 4, 2021 •

edited

Loading

pllim left a comment

pllim Feb 4, 2021

pllim Feb 4, 2021

pllim Feb 4, 2021

codecov bot commented Feb 5, 2021 •

edited

Loading

pllim commented Feb 5, 2021

pllim commented Feb 5, 2021

mperrin commented Feb 5, 2021

pllim commented Feb 5, 2021

shanosborne left a comment

pllim commented Feb 22, 2021

shanosborne commented Feb 22, 2021

pllim commented Feb 22, 2021

mperrin left a comment

mperrin Mar 29, 2021

pllim Mar 29, 2021

mperrin Mar 29, 2021

shanosborne Mar 29, 2021

pllim Mar 29, 2021

mperrin Mar 29, 2021 •

edited

Loading

mperrin commented Mar 29, 2021

pllim left a comment

pllim Mar 29, 2021

shanosborne commented Mar 29, 2021

shanosborne commented Mar 30, 2021

mperrin commented Apr 1, 2021

pllim commented Apr 1, 2021

		@@ -0,0 +1,26 @@
		# this contains imports plugins that configure py.test for astropy tests.

MNT: Port from pysynphot to stsynphot #388

MNT: Port from pysynphot to stsynphot #388

Conversation

pllim commented Feb 4, 2021 • edited Loading

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 5, 2021 • edited Loading

Codecov Report

pllim commented Feb 5, 2021

pllim commented Feb 5, 2021

mperrin commented Feb 5, 2021

pllim commented Feb 5, 2021

shanosborne left a comment

Choose a reason for hiding this comment

pllim commented Feb 22, 2021

shanosborne commented Feb 22, 2021

pllim commented Feb 22, 2021

mperrin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mperrin Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

mperrin commented Mar 29, 2021

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanosborne commented Mar 29, 2021

shanosborne commented Mar 30, 2021

mperrin commented Apr 1, 2021

pllim commented Apr 1, 2021

pllim commented Feb 4, 2021 •

edited

Loading

codecov bot commented Feb 5, 2021 •

edited

Loading

mperrin Mar 29, 2021 •

edited

Loading