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

Exploring_jwst_transmission #5

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

arjunsavel
Copy link

@arjunsavel arjunsavel commented Jun 12, 2022

Notebook to explore early JWST proposals for transmission spectroscopy of exoplanets. Currently just have the requirements.

NOTE now a notebook comparing stellar types — exoplanet spectra aren't available yet!

@kelle
Copy link

kelle commented Jun 18, 2022

Link to ReviewNB

@kelle kelle changed the title Add exploring_jwst_transmission notebook Exploring_jwst_transmission Jul 9, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@arjunsavel arjunsavel marked this pull request as ready for review September 23, 2022 05:20
@@ -0,0 +1,7074 @@
{
Copy link

@ttdu ttdu Sep 28, 2022

Choose a reason for hiding this comment

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

I think "Stellar Spectral Types" might make your the title more explicit


Reply via ReviewNB

@@ -0,0 +1,7074 @@
{
Copy link

@ttdu ttdu Sep 28, 2022

Choose a reason for hiding this comment

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

You should have some sort of introduction, even if it's similar to the other notebook. Each notebook should be "standalone", so don't worry about being having similar content in two notebooks. Consider: what astronomy background should people be familiar with to do this? Summarize and link to sources. What is your goal in this notebook? To compare features of an F-type star to G-type star, so let's state that here.

I agree with [link previously written SpectralDB notebook] :)

Once you work out the rest of introduction, you might want to add something like "For more information on using Specviz and SpectralDB", see the notebook [link]. I'll make sure it gets a link when we publish them all


Reply via ReviewNB

@@ -0,0 +1,7074 @@
{
Copy link

@ttdu ttdu Sep 28, 2022

Choose a reason for hiding this comment

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

It's okay to link to the other notebook in the introduction, but not throughout your own notebook. Adding links to documentation would be better. https://mast.stsci.edu/spectra/docs/


Reply via ReviewNB

@@ -0,0 +1,7074 @@
{
Copy link

@ttdu ttdu Sep 28, 2022

Choose a reason for hiding this comment

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

.gt: min_value (where gt stands for "greater than")


Reply via ReviewNB

@@ -0,0 +1,7074 @@
{
Copy link

@ttdu ttdu Sep 28, 2022

Choose a reason for hiding this comment

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

This cell should use the 'wavrange' variable you set up above


Reply via ReviewNB

@@ -0,0 +1,7074 @@
{
Copy link

@ttdu ttdu Sep 28, 2022

Choose a reason for hiding this comment

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

Since we're doing this twice, it's ok to jam everything together into one cell, as long as they retain their comments.

Also, why are we filtering this data for a SNR of >1, but the other data is >2? We should make them consistent or explain the need for them to be different


Reply via ReviewNB

@@ -0,0 +1,7074 @@
{
Copy link

@ttdu ttdu Sep 28, 2022

Choose a reason for hiding this comment

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

About the plots below:

  • Since we repeat the plotting steps three times, it might de-clutter things if we turn it into a function for ourselves
  • After each plot, there should be a little commentary. Do we see expected spectral features? Does this star look typical for its class? Is the data noisy? You don't need to talk about all of those things, but a small, relevant comment would be good

Reply via ReviewNB

@@ -0,0 +1,7074 @@
{
Copy link

@ttdu ttdu Sep 28, 2022

Choose a reason for hiding this comment

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

This was unexpected to me! (a recovering cosmologist)

This 'expectation' should be talked about in the intro, briefly, when you talk about the the end result you're aiming for. Ideally, with links to more information about stellar spectral classes.


Reply via ReviewNB

@@ -0,0 +1,7074 @@
{
Copy link

@ttdu ttdu Sep 28, 2022

Choose a reason for hiding this comment

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

A separate solutions notebook (doesn't need to pretty!) would be a nice addition


Reply via ReviewNB

@@ -0,0 +1,7074 @@
{
Copy link

@ttdu ttdu Sep 28, 2022

Choose a reason for hiding this comment

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

The first line in this cell should say:

If you have questions, comments, or feedback about this notebook, please email archive@stsci.edu.

You should not put your email on the notebook; you will get questions from users that MAST should be answering. Obviously this is a great notebook, and you should take pride in (and get credit for) your work. As an alternative, you could make your name a link to your website, if you have one.


Reply via ReviewNB

@kelle
Copy link

kelle commented Nov 16, 2022

@arjunsavel , Could you give this one more round of polish, taking into consideration Tom's comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants