-
-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New tutorials: Using synphot for synthetic photometry #384
base: main
Are you sure you want to change the base?
Conversation
c3c4c8a
to
807645d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Tiffany!
I think it is a policy to clear all outputs off the notebooks. And when you do, you would want to squash the commits so the notebooks with outputs are not in the history.
As for the notebook contents, they had went through review by mentors (myself, @bmorris3 , and @eteq ) during GSoC. Still, it is good to have extra pairs of eyes having a look.
with open(tmp_data_file, 'wb') as f: | ||
f.write(data) | ||
|
||
hdu = fits.open(tmp_data_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use fits.open()
as a context manager so the file handler can be closed on exit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble using fits for saving, since data
is a bytearray. For example, I try to replace lines 192 and 193 with
openhdu = fits.BinTableHDU(Table(data, dtype='bytearray'))
openhdu.writeto(tmp_data_file)
and get the value error ValueError: Data type <class 'bytes'> not allowed to init Table
. There's probably a better way to go about this that I'm not seeing... otherwise I could just add f.close()
?
(long time no see, btw! Hope you're doing relatively well in this brave new world)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 @tcjansen Where are you getting that error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, will need more info to reproduce the error on our side...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrn @kakirastern This happens in part 5 of the synphot-model-spec notebook, specifically in the call to my local version of skymodel.py
, where I'm testing using astropy.io.fits
to write the data received from a web request to a fits file and to close the handler on exit, as @pllim suggested (the way I have it in the current commit left the file open). I think the solution I proposed above would work fine if the requested data
weren't in bytes, since it appears astropy.table.Table
won't work with bytes (see the traceback below). Alternatively, I could just manually close the file with f.close()
at line 194 in skymodel
, unless there are objections to that.
I hope that clears up some confusion... let me know if I can provide more info! Here's the full traceback from my local version of skymodel.py
(which is only different by the two lines in question):
---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
<ipython-input-11-c6c183f1d026> in <module>
----> 1 trans_waves, transmission = skymodel.get_atmospheric_transmittance(airmass=1.5)
2
3 atmosphere = SpectralElement(Empirical1D,
4 points=trans_waves,
5 lookup_table=transmission)
/Volumes/external/github/astropy-tutorials/skymodel.py in get_atmospheric_transmittance(airmass, pwv_mode, season, time, pwv, msolflux, incl_moon, moon_sun_sep, moon_target_sep, moon_alt, moon_earth_dist, incl_starlight, incl_zodiacal, ecl_lon, ecl_lat, incl_loweratm, incl_upperatm, incl_airglow, incl_therm, therm_t1, therm_e1, therm_t2, therm_e2, therm_t3, therm_e3, vacair, wmin, wmax, wgrid_mode, wdelta, wres, lsf_type, lsf_gauss_fwhm, lsf_boxcar_fwhm, observatory, keep_tmp_data_file)
201 # with open(tmp_data_file, 'wb') as f:
202 # f.write(data)
--> 203 openhdu = fits.BinTableHDU(Table(data, dtype='bytearray'))
204 openhdu.writeto(tmp_data_file)
205
~/anaconda3/lib/python3.7/site-packages/astropy/table/table.py in __init__(self, data, masked, names, dtype, meta, copy, rows, copy_indices, **kwargs)
514 else:
515 raise ValueError('Data type {0} not allowed to init Table'
--> 516 .format(type(data)))
517
518 # Set up defaults if names and/or dtype are not specified.
ValueError: Data type <class 'bytes'> not allowed to init Table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember much from GSoC last year anymore. Did this ever work before? What if you do data = response.text
and avoid bytes?
"metadata": {}, | ||
"source": [ | ||
"<a id=\"qe\"></a>\n", | ||
" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... Did we discuss why it was 51% difference? Seems a little high.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pllim I believe we did discuss it - since we are comparing the model counts to real counts Brett took from the ground on a specific night, I think the consensus we reached was that it likely has to do with whatever the observing conditions were that night, especially since the error of the other ground-based count prediction (of HAT-P-11) agrees with the predicted space-based count errors (all less than %15).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we did, but I can't say I remember. Maybe @bmorris3 does.
"metadata": {}, | ||
"source": [ | ||
"<a id=\"qe\"></a>\n", | ||
" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like there's a bug here which is making the CircleCI fail:
" | |
" kepler_stars[star]['observation'] = Observation(kepler_stars[star]['spectrum'], kepler_response)" |
Pinging @tcjansen once more about this PR 😄 |
Hi @tcjansen Would you like me to push changes to your branch to resolve the issues/conflicts needed to be addressed, or would you like to make these changes yourself? I understand @bmorris3 really would like to see this work gets merged and published to Learn Astropy soon. Anyway, please let me know. |
Absolutely! I would also really like to get this merged. Thanks for bringing it back to my attention, I will start working on it this week. |
Sure @tcjansen Do let me know if I could be of any help. Also, remember to rebase against the |
👋 We have changed the build infrastructure and layout of files in the repo, so I am going to rebase this pull request and force push to your branch. If you have any local changes that haven't been pushed, please push them by the end of this week (October 1). After I have rebased to bring the new build machinery in, please let us know if you would like to continue working on this tutorial or if you would rather someone else take it over. Thanks! |
@adrn Hi, thanks for the update/nudge! Looks like this fell off my radar because I just could not understand why the ci/circleci check is failing 😅... If anyone has any suggestions I would be happy to take another look, otherwise I wouldn't mind if someone else wanted to take it over. |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Cool! I just rebased to pull in the latest changes to the infrastructure (hopefully that will fix the failing build!), so you'll need to |
Hi all, I'm a GSoC student working with @bmorris3, @pllim, and @eteq. During the early stages of my project I developed some example use cases for synphot that I think could also be helpful for the astronomical community. In this tutorial we show how to use synphot to predict photometric fluxes of an SDSS observation, and I plan on submitting another tutorial for creating synthetic photometry using model spectra.
This is my first PR to astropy-tutorials, so suggestions are very welcome!
Fixes #424