Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add tutorial notebook for TIKE #149
Add tutorial notebook for TIKE #149
Changes from 7 commits
81213f9
16e51f1
199b0f9
5e57f5e
f64f33f
776e0b2
6f6f7e8
abd0ab8
1c83a13
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
lcviz
isn't currently installed on the production version of TIKE – I'm assuming that will be done when this notebook is available?Also, these imports only work if you're in the kernel named
TESS Environment
. Should we mention that here?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.
Correct, they will install it once 1.0 is released AND they have this notebook. I think the general instructions handle selecting the kernel environment, but that's a good question for @ttdu
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.
lcviz will be installed in ~December, when we release a new environment for AAS.
we normally specify the default kernel in the notebook metadata, so that
TESS Environment
is automatically selected. it never hurts to be verbose, but Kyle is correct that the general instructions cover thisThere 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.
since this mentions TIKE specifically, maybe we don't want to actually merge this PR here?
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.
what could we do to make this more seamless (maybe not in time for lcviz 1.0, but in the future)? Could we have a switch in lcviz to do both of these? (cc @bmorris3)
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.
This might be something it would be nice to do upstream - have a switch in Jdaviz at the app or helper level to turn on cloud support (with optional dependencies) that would then allow
load_data
to take a cloud URI.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.
would also be nice to be able to get this directly from lightkurve, but I'm guessing that isn't possible?
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 believe so.
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.
you could shorten this by passing everything into
get_cloud_uris
e.g. this JWST query
becomes a single (but more complicated) line of code:
cloud_files = Observations.get_cloud_uris(objectname="NGC 3132", obs_collection="JWST", filter_products={"productType": "SCIENCE", "calib_level": [3]})
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.
@bmorris3 - what would it take to be able to extend the upstream URI capability to be able to pass
lc_uri
toload_data
directly?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.
It would require one more little block of logic in the jdaviz URI parser. That's work we need to do for RSP soon anyway. In their notebooks, they use this pattern on AWS:
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've tried running the notebook as-is on TIKE and it doesn't work yet, for reasons I don't understand. One challenge that we're likely to face is that lightkurve has tools for initializing a
LightCurve
from a file path to a FITS file, but not from an open file stream to an HDU list, which is whatfsspec
is likely to give us.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.
this isn't technically public API... can this be pulled out of
lcviz.get_data(...).meta
instead?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.
Probably, I just pulled this from the LCvizExample notebook. Should probably change it there too if we don't want people using this.
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.
Ok, I'd vote to do that if possible to avoid publicizing any internal API if there are alternatives
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.
can we avoid using
Time
entirely or is this needed because reference time is defined differently? Or is it good to show how to do this here?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.
Again, I just pulled this from the example notebook, which I think @bmorris3 wrote. Probably a question for him.
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.
Why would you want to avoid using
Time
? Time coordinates are dangerously ambiguous otherwise.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.
because currently it needs to be passed to lcviz just as a float anyways, so creating a time object just to do
to_value
seems to add complication to the notebook. Maybe we just link to docs on Time (whether or not we use it), since we don't want this to be a tutorial on that whole topic 🤷♂️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 working through the notebook, and I think as long as the time system is written in the comment, I'm ok with avoiding
astropy.time.Time
here. The most likely "gotcha" to worry about is that the units of the reference epoch, the period, and the time axis of the light curve must all be in the same units. If you had period in hours, this wouldn't work correctly. For this example, we're fine without specifying.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.
not sure if this link will break in the future when they fix
docs.lightkurve.org
🤔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.
@kecnry – should we make a zenodo record for lcviz? (I vote yes!)
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.
yes, let's plan to do that with 1.0 release. I'll add it to the ticket.