Create plot_lightcurves notebook #19
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
| @@ -0,0 +1,1678 @@ | |||
| { | |||
There was a problem hiding this comment.
Yes, you should walk through this code. In-line comments are nice, but markdown cells between the code cells help the reader follow what you're doing
Reply via ReviewNB
| @@ -0,0 +1,1678 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #7. "User-agent":"python-requests/"+version}
In our testing, this line seems unnecessary; you don't have to specify the python version. Try removing this line, and if your code still functions, we should take this out to simplify the header and the required imports
Reply via ReviewNB
| @@ -0,0 +1,1678 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #4. 'radius':0.2},
Search radius should be 5arcsec; will be faster, large radius not needed for exoplanets
Reply via ReviewNB
| @@ -0,0 +1,1678 @@ | |||
| { | |||
There was a problem hiding this comment.
This should maybe be a "helper function"; we want users to understand what happens, and we should explain what goes on here, but converting a JSON to astropy table is a "sweep it under the rug" step of the process. Basically just do something like:
def JSON2aptable(): function goes here
And explain to users that we're just converting between data types
Reply via ReviewNB
There was a problem hiding this comment.
Great point! @kelle on previous notebooks you'd indicated that I might not want to introduce functions, because users might not be familiar with them. What are your thoughts here?
| @@ -0,0 +1,1678 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #1. sci_prod_arr = [x for x in obs_products['data'] if x.get("productType", None) == 'SCIENCE']
Can we filter for science products from the beginning? That would make it easier to look through the results
Reply via ReviewNB
There was a problem hiding this comment.
Hmm, poking around it seems like I can't incorporate this into the actual request. I'll take a closer look at the documentation, though!
| @@ -0,0 +1,1678 @@ | |||
| { | |||
There was a problem hiding this comment.
Why do we expect this to be cleaner? We should be explicit about this when we explain PDCSAP_Flux.
Reply via ReviewNB
| @@ -0,0 +1,1678 @@ | |||
| { | |||
There was a problem hiding this comment.
Does this make it easier to see data? Or are you just trying to create a "publication-ready" plot? Either way, you should explain why you want to do this
Reply via ReviewNB
| @@ -0,0 +1,1678 @@ | |||
| { | |||
There was a problem hiding this comment.
Spitzer is a mess. I don't think it has any files listed as "timeseries"
Limb-darkening/atmospheric absorption would be interesting but somewhat complicated here. Perhaps as an exercise for the reader, to be included at the bottom of the notebook?
Reply via ReviewNB
| @@ -0,0 +1,1678 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,1678 @@ | |||
| { | |||
There was a problem hiding this comment.
Re: comment on the other notebook, we don't want questions being redirected to you. "For support contact archive@stsci.edu"
Reply via ReviewNB
|
@arjunsavel , please incorporate Tom's comments. |
Based on a comment made in our MAST developers meeting, I've made a short notebook describing how to plot exoplanet light curves from two different sources: the MAST API and
lightkurve.There's a few pending questions about scope (e.g., whether this should be its own notebook to begin with!), but if this notebook seems like it's going in the right direction, I can get going on adding exercises.