WIP - A higher order function for wrapping synphot functionality #207

tcjansen · 2019-07-30T15:52:37Z

The initial state of this PR is beyond a work in progress if you know what I mean 😅, but I wanted to put it out there so we can chat about it in our GSoC meeting.

So I'm envisioning some kind of wrapper for synphot in which you can initiate a class with the source spectrum of an object, give a telescope diameter, and boom, you can get attributes like SNR, countrate, exptime, etc...:

>>> from synphot.spectrum import SourceSpectrum
>>> from synphot.wrapper import Observe
>>> import astropy.units as u

>>> tele_diameter = 1 * u.m
>>> source_spec = SourceSpectrum(...)
>>> some_observation = Observe(source_spec, tele_diameter)

>>> some_observation.countrate
1e6 ct/s
>>> some_observation.snr
100
...

The observation can be customized by setting a bandpass, giving a desired SNR (to get an exposure time), providing the quantum efficiency of the CCD or an atmospheric transmission model, etc.

synphot/tests/test_wrapper.py

synphot/wrapper.py

bmorris3 · 2019-07-30T17:35:12Z

synphot/wrapper.py

+    def __init__(self, source_spec, diameter, distance=0 * u.pc,
+                 bandpass=SpectralElement(Const1D, amplitude=1),
+                 quantum_efficiency=None, atmospheric_transmission=None,
+                 force=None, exptime=1 * u.s):


Should things like exptime have a default values? I think that's a thing that will really need to be supplied in every case, right? And distance sticks out as bing an odd one too, nothing you observe is actually at 0 pc.

Sure, I agree with exptime not being a default param.

As for distance, I set the default to 0 instead of something like False for a couple of reasons... first as a way to indicate that by default Expose assumes the user has already scaled the source spectrum for distance (i.e. source_spec should be as it is just before it hits the instrument, i.e. 0 pc), and also because while scaling the spectrum for a star is straight forward ([R / d]^2), I wasn't sure how exactly to do that for something like a galaxy so I put it off for the moment 😂

I guess I was envisioning that the user could give the source spectrum as it is at the object, and Expose could scale the spectrum given a distance. But on second thought, I think it would be better for all parties if we get rid of distance and just specify that Expose assumes the user has already scaled the spectrum.

synphot/wrapper.py

bmorris3 · 2019-07-30T17:40:33Z

synphot/wrapper.py

+    @u.quantity_input(diameter=u.m, distance=u.pc, exptime=u.s)
+    def __init__(self, source_spec, diameter, distance=0 * u.pc,
+                 bandpass=SpectralElement(Const1D, amplitude=1),
+                 quantum_efficiency=None, atmospheric_transmission=None,


I think this might be controversial (👀 @eteq), but I might vote for atmospheric transmission to take a boolean input, and if True, return the skycalc transmittance curve with some default parameters; if False, don't include atmospheric transmission. I think for 90% of users that will give them the answer they want within a factor of 2 with as little data wrangling as possible.

I might even vote the same thing for quantum_efficiency. Perhaps it could instead be silicon_detector=True, and if True, return the generic CCD response curve that we have on hand. If False, assume a constant throughput.

Okay, I agree with this. Though I am curious what the controversy is..!

synphot/wrapper.py

bmorris3 · 2019-07-30T17:54:32Z

synphot/wrapper.py

+            raise exceptions.BandpassError("quantum_efficiency should be "
+                                           "a list with 2 x n "
+                                           "dimensions.")
+        try:


Reminder RE: slack convo to let the function fail when the user supplies the wrong inputs, rather than writing the same exception block in a bunch of places. Bumpers are nice but at some point obfuscate your code.

Since I've moved these bits to a function (where the exception is only stated once) it's okay to keep?

bmorris3 · 2019-07-30T17:55:30Z

synphot/wrapper.py

+
+        self.throughput *= self.atmospheric_transmission
+
+@u.quantity_input(waves=u.angstrom, frac=u.Unit(''))


I don't think a private method needs a decorator to enforce that you're using it correctly. Ideally the user will never know this function exists, so they shouldn't need forceful reminders to get their units correct.

Ah yeah, I put that decorator where it is because there doesn't seem to be a better place to put it. Passing these parameters in as lists is not the default option, so I couldn't put it before __init__(), but I do still want to enforce that the user's custom models are in the correct units.

synphot/wrapper.py

bmorris3 · 2019-07-30T17:59:50Z

synphot/wrapper.py

+                "list of shape(2, n) containing "
+                "`~astropy.units.quantity.Quantity` values")
+
+        self.throughput = self.bandpass


I might be missing something here, but is there a point of having a separate throughput and bandpass object carried around if one becomes the other eventually?

This comes from me wanting the user to be able to call self.bandpass to inspect the filter, while self.throughput would allow them to see all of the throughput effects in play (bandpass, QE, atmosphere, etc).

synphot/wrapper.py

pllim · 2019-07-31T03:05:32Z

Thanks! I don't have a chance to do a detailed review yet, but I would propose renaming wrapper.py to something less cryptic. How about groundbased or something to explain at a high level the intent of your new module?

tcjansen · 2019-08-01T05:53:27Z

Does expose.py work? It follows some other synphot style choices as well (e.g. synphot.observe.Observe)

pllim

Thank you for your contribution!

Re: Naming things -- How about groundbased.py and rename the class to Exposure?

Before I could review this, this PR needs to be cleaned up to not include changes from #204. There is no easy way since you built this branch off your other unmerged branch. One way I can think of is to:

Make a backup of this branch, just in case.
Copy the files you want to keep for this PR off somewhere outside the Git repo (e.g., HOME directory).
git fetch upstream master
git reset --hard upstream/master (this will bring your branch back in sync with master here but you will lose all your changes)
Copy those files you moved away in Step 2 back into the branch. Add and commit them.
git push -f origin higherfunc

Test failures caused by unused imports and variables, and using a new dependency that is not declared in setup nor test setup script. I would suggest you run the tests locally first, and fix the failures, before pushing commits to GitHub. You can run the following:

flake8 synphot
python setup.py test --remote-data --open-files

In regards to the new requests dependency, why do we need it here? Please see comment.

CHANGES.rst

pllim · 2019-08-05T20:04:44Z

synphot/expose.py

+                         'skycalc_cli documentation.')
+
+    # Use the bit from skycalc_cli which queries from the SkyCalc Sky Model
+    server = 'http://etimecalret-001.eso.org'


Why is this not an astroquery call?

That bit is an artifact from me borrowing straight from skycalc_cli. Is there an astropy equivalent to requests?

There is a astropy.utils.data.download_file() function but I think @bmorris3 said that it is not quite what you need. So my other suggestion would be to move the skycalc query out to its own module (after you created this "experimental" folder and move your work there).

tcjansen · 2019-08-06T03:23:23Z

So I included changes from #204 because this PR requires those changes in order to run properly (to call ccd_snr for example). Is it not a problem to leave those changes out?

tcjansen · 2019-08-06T03:26:28Z

As for the name changes, maybe exposure.py and Exposure? groundbased.py would be misleading because it doesn't actually require atmospheric transmission.

pllim · 2019-08-06T15:24:27Z

@tcjansen

How about we make a new "experimental" folder under "synphot" and then you can name it exposure.py in that folder? In the future, when the API matures, I can rethink on where to house it permanently.
Are you suggesting that we merged Adding signal to noise ratio calculations to observation.py #204 into this PR? If so, please close Adding signal to noise ratio calculations to observation.py #204 without merge.

Thanks!

…and a sky model

pllim · 2019-08-13T19:43:01Z

As per the telecon today, @tcjansen will move these new functionalities to a new subpackage in astroplan instead (no need to put in experimental folder there), with synphot being an optional dependency over there. cc @bmorris3 and @eteq

Thank you for your continued patience, Tiffany!

pllim · 2019-08-21T14:44:19Z

Since the astroplan PR is live, I am closing this. Thanks again, Tiffany!

tcjansen force-pushed the higherfunc branch from 8a8004f to 8a060c1 Compare July 30, 2019 16:06