Skip to content
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

Closed
wants to merge 9 commits into from

Conversation

rosteen
Copy link
Contributor

@rosteen rosteen commented Oct 21, 2024

I figured I would open a PR here first to get comments, and then we can move/copy the notebook to wherever TIKE needs it for review.

@rosteen rosteen added the documentation Improvements or additions to documentation label Oct 21, 2024
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.20%. Comparing base (7b78df5) to head (1c83a13).
Report is 47 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #149      +/-   ##
==========================================
- Coverage   93.92%   91.20%   -2.73%     
==========================================
  Files          37       41       +4     
  Lines        1598     2149     +551     
==========================================
+ Hits         1501     1960     +459     
- Misses         97      189      +92     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rosteen rosteen added this to the 1.0.0 milestone Oct 22, 2024
notebooks/tike_lcviz_tutorial.ipynb Outdated Show resolved Hide resolved
notebooks/tike_lcviz_tutorial.ipynb Outdated Show resolved Hide resolved
notebooks/tike_lcviz_tutorial.ipynb Outdated Show resolved Hide resolved
notebooks/tike_lcviz_tutorial.ipynb Outdated Show resolved Hide resolved
"source": [
"## Loading light curves into LCviz\n",
"\n",
"Because TIKE already runs in the cloud, it is fastest and easiest to load data that is also hosted in the cloud using the `s3fs` package, which allows you to access cloud data as if it were local. Here we will use [astroquery](https://astroquery.readthedocs.io/en/latest/) to get the URI for the file that we want to load. For more information about the code below, see [this TIKE webinar notebook](https://github.com/spacetelescope/tike_content/blob/main/content/notebooks/webinar-series/01-lightcurves/01-Lightcurves.ipynb).\n"
Copy link
Member

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?

Comment on lines +224 to +225
"light_curve = lightkurve.read(lc_uri)\n",
"lcviz.load_data(light_curve)"
Copy link
Member

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 to load_data directly?

Copy link
Contributor

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:

import s3fs
import roman_datamodels as rdm

asdf_dir_uri = 's3://roman-sci-test-data-prod-summer-beta-test/'
fs = s3fs.S3FileSystem()

asdf_file_uri = asdf_dir_uri + 'ROMANISIM/DENSE_REGION/R0.5_DP0.5_PA0/r0000101001001001001_01101_0001_WFI16_cal.asdf'
with fs.open(asdf_file_uri, 'rb') as f:
    file = rdm.open(f)

Copy link
Contributor

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 what fsspec is likely to give us.

Comment on lines 284 to 286
"# get the origin of the time axis in LCviz:\n",
"time_coordinates = lcviz.app.data_collection[0].coords\n",
"reference_time = time_coordinates.reference_time\n",
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

"reference_time = time_coordinates.reference_time\n",
"\n",
"# literature ephemeris for hot Neptune planet HAT-P-11 b:\n",
"morris2017_epoch = Time(2454605.89146, format='jd')\n",
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Member

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 🤷‍♂️

Copy link
Contributor

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.

"\n",
"- [LCviz documentation](https://lcviz.readthedocs.io/en/stable/index.html)\n",
"- [LCviz Github](https://github.com/spacetelescope/lcviz)\n",
"- [lightkurve documentation](https://lightkurve.github.io/lightkurve/)\n",
Copy link
Member

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 🤔

notebooks/tike_lcviz_tutorial.ipynb Outdated Show resolved Hide resolved
Copy link
Contributor

@bmorris3 bmorris3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending support from Thomas to make the file loading work, this looks good!

notebooks/tike_lcviz_tutorial.ipynb Outdated Show resolved Hide resolved
"reference_time = time_coordinates.reference_time\n",
"\n",
"# literature ephemeris for hot Neptune planet HAT-P-11 b:\n",
"morris2017_epoch = Time(2454605.89146, format='jd')\n",
Copy link
Contributor

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.

notebooks/tike_lcviz_tutorial.ipynb Outdated Show resolved Hide resolved
"* [Citing `astropy`](https://www.astropy.org/acknowledging.html)\n",
"* [Citing `lightkurve`](http://docs.lightkurve.org/about/citing.html)\n",
"\n",
"And cite Jdaviz through its [Zenodo record](https://doi.org/10.5281/zenodo.5513927).\n"
Copy link
Contributor

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!)

Copy link
Member

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.

}
},
"source": [
"## Imports\n",
Copy link
Contributor

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?

Copy link
Member

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

Copy link

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 this

Comment on lines +224 to +225
"light_curve = lightkurve.read(lc_uri)\n",
"lcviz.load_data(light_curve)"
Copy link
Contributor

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 what fsspec is likely to give us.

@kecnry
Copy link
Member

kecnry commented Dec 2, 2024

moved to spacetelescope/tike_content#46

@kecnry kecnry closed this Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation no-changelog-entry-needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants