Adding synphot #424

shanosborne · 2021-03-01T16:17:24Z

This PR aims to work with spacetelescope/poppy#388 to replace pysynphot with stsynphot.

After discussion with @mperrin, we decided that completely replacing pysynphot in webbpsf would cause problems for our dev users (especially ote sim mirage users) if they have mismatched webbpsf and poppy versions. So instead, we decided to add a check for the user's poppy version inside webbpsf. If poppy > 0.9.2 (the latest release), the code would use stsynphot, if poppy <= 0.9.2, the code would use pysynphot. We will remove this check for the next release of webbpsf, but this will help users in the meantime.

Testing Note: Because of the version check above, since the poppy PR with the stsynphot changes isn't merged yet, in order to test this you'll need to change the > to < in line 57 in webbpsf_core.py. That or we'll need to rebase the poppy PR to appear that the change is after 0.9.2

shanosborne · 2021-03-01T16:17:57Z

@pllim I've tried running this branch with your branch in poppy and I'm getting some errors from the stsynphot calls in poppy. Would you be able to look this over and see if you understand the errors? Thanks!

pllim

pysynphot and stsynphot are not 100% interchangable, especially when it comes to Quantity support and underlying machinery.

pllim · 2021-03-01T16:39:00Z

dev_utils/compute_psf_library.py

+if package_version.parse(poppy_ver) > package_version.parse("0.9.2"):
+    import stsynphot
+else:
+    import pysynphot


I wouldn't do this: This looks like a maintenance nightmare. Much easier to cut the next major release and not use pysynphot at all.

Agreed in the long run, yes. This is intentional and needed in the short term transitional period however. We plan to take it out after this next release.

codecov · 2021-03-01T17:13:03Z

Codecov Report

Merging #424 (2692bbf) into develop (ce518be) will decrease coverage by 0.00%.
The diff coverage is 50.45%.

❗ Current head 2692bbf differs from pull request most recent head 9ac51a9. Consider uploading reports for the commit 9ac51a9 to get more accurate results

@@             Coverage Diff             @@
##           develop     #424      +/-   ##
===========================================
- Coverage    48.39%   48.39%   -0.01%     
===========================================
  Files           14       14              
  Lines         5837     5912      +75     
===========================================
+ Hits          2825     2861      +36     
- Misses        3012     3051      +39

Impacted Files	Coverage Δ
webbpsf/obssim.py	`0.00% <0.00%> (ø)`
webbpsf/jupyter_gui.py	`7.97% <38.46%> (+2.32%)`	⬆️
webbpsf/utils.py	`61.56% <57.89%> (-0.23%)`	⬇️
webbpsf/distortion.py	`81.81% <60.00%> (-13.42%)`	⬇️
webbpsf/webbpsf_core.py	`80.58% <63.63%> (+0.69%)`	⬆️
webbpsf/roman.py	`90.00% <0.00%> (+0.22%)`	⬆️

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 ad9c2de...9ac51a9. Read the comment docs.

pllim · 2021-03-01T17:15:39Z

The tests passed here. I don't understand where are the errors you need help with. Can you please clarify? Thanks!

shanosborne · 2021-03-01T18:04:29Z

@pllim, the errors come from running this branch of webbpsf with your pysyn-stsyn-port branch of poppy, in an environment where stsynphot is installed

pllim · 2021-03-01T18:06:21Z

@shanosborne , can you please have this branch call that branch here, so we can all see the log in CI? Thanks!

pllim · 2021-03-01T20:08:03Z

I waited a while for https://travis-ci.org/github/spacetelescope/webbpsf/jobs/760994800 to run but it passed. 🤷 (I cancelled the irrelevant jobs.)

shanosborne · 2021-03-01T20:11:29Z

Oh shoot you're right. I messed up the test and didn't add an stsynphot installation to it, so it passed because it never called any of the stsynphot code (which is optional and is only called if you have the package installed). I've just updated the test and hopefully it actually tests what we want it to now. Sorry about that!

pllim · 2021-03-01T20:35:42Z

Seems like Travis CI has a very long queue time now. Still waiting for it to start. 🤞

shanosborne · 2021-03-01T21:06:53Z

Right, so now we have the case where the fact that the poppy PR isn't merged yet has become a problem. So I'm going to add a commit to flip the sign of the version check in webbpsf_core.py which we will need to undo before merging the PR, but is necessary for testing.

mperrin · 2021-03-01T21:27:17Z

@shanosborne for the version check - how about instead of looking at the version number we try looking at just what poppy is using directly. Something like this:

try:
   import poppy.instrument.stsynphot
   # poppy is using sysynphot so webbpsf should too
   import stsynphot
except ImportError:
   import pysynphot

And, optional, but I wonder if it's worth consolidating these if/else statements into one place in the code, so there's less to undo later when we can take out all the pysynphot refs eventually.

This all would indeed be easier if we could afford a case of temporary breakage of dependencies if we don't merge both PRs at the exact same time, but alas from past experience that way lies confusion and hassle...

pllim · 2021-03-01T21:33:39Z

I still think it is less maintenance burden to branch out 0.9.x that only uses pysynphot and only support stsynphot going forward. People who want to use pysynphot can use your 0.9.x releases and you can backport things into it from develop as needed. But I am not the one who has to maintain this package.

pllim · 2021-03-01T21:57:07Z

I guess we finally got it? Are the errors in https://travis-ci.org/github/spacetelescope/webbpsf/jobs/761016058 what you also see locally, @shanosborne ?

pllim · 2021-03-01T22:02:39Z

Most of the failures are caused by your source and bandpass "waveset" not overlapping completely or at all. This usually means either one of them have wavelength in existing values but now in wrong unit or some assumptions you made with PySynphot no longer makes sense. I'll see if I can sniff it out with your test cases.

There is one error where somehow you are passing in zero or negative for wavelength, which is not allowed.

pllim · 2021-03-04T21:28:41Z

After discussions with @shanosborne , I have created this gist to explain why you are seeing failures, and provide workarounds, where possible. Many thanks to Shannon who patiently provided me with the necessary info.

tl;dr

No overlap and partial overlap -- Your bandpass is too far from the blackbody peak, resulting in useless observation. You can force the observation to happen in stsynphot by resampling the blackbody (as shown in the gist above) but don't expect the observation to give you anything useful.
~~Negative wavelength -- Your bandpass is unphysical. Do not create such a bandpass.~~ After the bandpass typo is fixed, this is also a partial overlap case, as above.

mperrin · 2021-04-01T02:52:32Z

@shanosborne FYI the PR in poppy is now merged, so here we can take out the temporary parts (extra tox test case etc) and hopefully merge this shortly. Can you take care of the few updates needed in this, please? I'm not sure if anything's needed besides just the changes in the test setup?

mperrin

As discussed this should have the temporary test case revised out before we merge.

Looking at this now I'm starting to have second thoughts about the back compatibility approach, but probably not enough to revise. I still want to make sure we take zero chance of breaking any dependencies in any of the team practices etc; but I'm not sure we want all the switching code around in the long run. We probably should consider pysynphot deprecated and remove this before some later release I suppose.

One minor change requested, plus the test case stuff, and then this should be good to merge. Thanks!

mperrin · 2021-04-01T02:57:22Z

docs/references.rst

-Pysynphot CDBS files used for the development version of the JWST Exposure Time Calculators (ETCs),
+Stsynphot CDBS files used for the development version of the JWST Exposure Time Calculators (ETCs),
 normalized to peak transmission = 1.0 (because absolute throughput is not
-relevant for PSF calculations). Not all filters are yet supported in Pysynphot,
+relevant for PSF calculations). Not all filters are yet supported in Stsynphot,
 however.


Minor: This text can be updated, as it's outdated in ways besides just the py/stsynphot change. Let's say the following:

Instrumental relative spectral responses are derived from the reference data files used by the Pandeia engine <https://jwst-docs.stsci.edu/jwst-exposure-time-calculator-overview/jwst-etc-pandeia-engine-tutorial/installing-pandeia#InstallingPandeia-DataFiles>_ for the JWST Exposure Time Calculator<https://jwst.etc.stsci.edu>_, normalized to peak transmission=1.0 (because absolute throughput is not relevant for PSF calculations).

…nto stsynphot

pllim · 2021-04-01T14:26:58Z

Re: tox -- You still want a new job to test against stsynphot (unless you want to swap it out with pysynphot in existing jobs) but maybe not call it "poppydev".

Update dependencies min versions

…to shanosborne-stsynphot

mperrin · 2021-04-01T22:37:20Z

Travis passes, codecov is "failing" but irrelevant (-0.01% delta), I'm going ahead with this since it's needed with the current dev poppy.

shanosborne added 5 commits March 1, 2021 10:16

Add stsynphot calls alongside pysynphot

a48ec12

missed file with pysynphot

dc92375

finishing missed file

21a1b11

add tox test and update docs

ad93c89

flip sign

6540568

pllim reviewed Mar 1, 2021

View reviewed changes

Add temporary test to CI for poppy pllim/pysyn-stsyn-port branch

465221a

add stsynphot to new test

e2deb6f

flip version check for testing - must be undone later!

b9e5fe3

This comment has been minimized.

Sign in to view

Re-do checks

be6e4f6

shanosborne requested a review from mperrin March 9, 2021 20:55

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

mperrin requested changes Apr 1, 2021

View reviewed changes

remove test case and update docs

8b34c28

shanosborne added 2 commits April 1, 2021 08:53

Merge branch 'develop' of https://github.com/spacetelescope/webbpsf i…

2692bbf

…nto stsynphot

Update deps min versions to match poppy

caf33c5

mperrin added 3 commits April 1, 2021 16:11

Merge pull request spacetelescope#436 from shanosborne/update-deps

acd1529

Update dependencies min versions

Merge branch 'stsynphot' of https://github.com/shanosborne/webbpsf in…

4cfa7de

…to shanosborne-stsynphot

swap stsynphot -> synphot, to avoid GB of exteraneous downloads

9ac51a9

mperrin changed the title ~~Adding stsynphot~~ Adding synphot Apr 1, 2021

mperrin merged commit 7ac2b7c into spacetelescope:develop Apr 1, 2021

shanosborne deleted the stsynphot branch July 4, 2021 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding synphot #424

Adding synphot #424

shanosborne commented Mar 1, 2021

shanosborne commented Mar 1, 2021

pllim left a comment

pllim Mar 1, 2021 •

edited

Loading

mperrin Mar 1, 2021

codecov bot commented Mar 1, 2021 •

edited

Loading

pllim commented Mar 1, 2021

shanosborne commented Mar 1, 2021

pllim commented Mar 1, 2021

pllim commented Mar 1, 2021 •

edited

Loading

shanosborne commented Mar 1, 2021

pllim commented Mar 1, 2021

shanosborne commented Mar 1, 2021

mperrin commented Mar 1, 2021

pllim commented Mar 1, 2021

pllim commented Mar 1, 2021

pllim commented Mar 1, 2021 •

edited

Loading

This comment has been minimized.

pllim commented Mar 4, 2021 •

edited

Loading

mperrin commented Apr 1, 2021

mperrin left a comment

mperrin Apr 1, 2021

pllim commented Apr 1, 2021

mperrin commented Apr 1, 2021

Adding synphot #424

Adding synphot #424

Conversation

shanosborne commented Mar 1, 2021

shanosborne commented Mar 1, 2021

pllim left a comment

Choose a reason for hiding this comment

pllim Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

mperrin Mar 1, 2021

Choose a reason for hiding this comment

codecov bot commented Mar 1, 2021 • edited Loading

Codecov Report

pllim commented Mar 1, 2021

shanosborne commented Mar 1, 2021

pllim commented Mar 1, 2021

pllim commented Mar 1, 2021 • edited Loading

shanosborne commented Mar 1, 2021

pllim commented Mar 1, 2021

shanosborne commented Mar 1, 2021

mperrin commented Mar 1, 2021

pllim commented Mar 1, 2021

pllim commented Mar 1, 2021

pllim commented Mar 1, 2021 • edited Loading

This comment has been minimized.

pllim commented Mar 4, 2021 • edited Loading

mperrin commented Apr 1, 2021

mperrin left a comment

Choose a reason for hiding this comment

mperrin Apr 1, 2021

Choose a reason for hiding this comment

pllim commented Apr 1, 2021

mperrin commented Apr 1, 2021

pllim Mar 1, 2021 •

edited

Loading

codecov bot commented Mar 1, 2021 •

edited

Loading

pllim commented Mar 1, 2021 •

edited

Loading

pllim commented Mar 1, 2021 •

edited

Loading

pllim commented Mar 4, 2021 •

edited

Loading