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 Specviz JWebbinar notebook #12

Merged
merged 6 commits into from
May 4, 2021

Conversation

rosteen
Copy link
Contributor

@rosteen rosteen commented Apr 23, 2021

I tried to be fairly detailed in the notebook, in case people who can't make the live session later get access to the notebook.

Note that this currently uses an SDSS spectrum, as I haven't gotten a nice JWST file to use yet.

@rosteen
Copy link
Contributor Author

rosteen commented Apr 23, 2021

I also imagine I should probably host the file I'm using on Box, right now it pull straight from SDSS.

@camipacifici
Copy link
Collaborator

Thank you!
Yes, files should be on box. You should have received the invite to the correct folder.

@rosteen
Copy link
Contributor Author

rosteen commented Apr 26, 2021

Added the file to the 2_jdat folder where Duy put their data. I created a link and made it viewable by anyone with the link, but when using this code in the notebook:

fn = download_file('https://stsci.box.com/s/pu9vh5y2hw0gz984izvkc8jrzy8qj795')
specviz.load_spectrum(fn, data_label="Demo SDSS", format="SDSS-III/IV spec")

I get OSError: Empty or corrupt FITS file. Any clue as to where I might have gone wrong in the Box setup? I can download the file manually from Box and confirmed that astropy.io.fits can open it.

@cslocum
Copy link
Contributor

cslocum commented Apr 26, 2021

image

@rosteen
Copy link
Contributor Author

rosteen commented Apr 26, 2021

@cslocum that particular error message is something I can fix, but it should only happen if you try to load the same data twice. Can you confirm that that's the case where you saw the error?

@rosteen
Copy link
Contributor Author

rosteen commented Apr 26, 2021

Created a PR in jdaviz to fix that: spacetelescope/jdaviz#568

Copy link
Collaborator

@eteq eteq left a comment

Choose a reason for hiding this comment

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

After some out-of-band discussion @camipacifici and I realized we want a requirements.txt so that people who grab this notebook outside of the webbinar know what they need to have installed. So can you add a requirements.txt a la ac38da0 ?

(It will conflict with the other PRs but there's no helping that, so we'll just have to merge-as-we-go 🤷 )

@camipacifici
Copy link
Collaborator

Looks great! I really like the idea of having beginner and advanced for the exercises.
Same comment as for @eteq to use JWST simulated data or some model here. @PatrickOgle might have something for you.

@cslocum
Copy link
Contributor

cslocum commented Apr 26, 2021

@cslocum that particular error message is something I can fix, but it should only happen if you try to load the same data twice. Can you confirm that that's the case where you saw the error?

I did run it twice. The first time I selected "run all cells", and after the kernel indicated it was idle, I noticed that that particular cell had not been run. So I ran just that one and got the error.

I suggest maybe adding a check in there to see if the file already exists? (or at least I think that would fix it/make it more robust.)

@camipacifici
Copy link
Collaborator

@rosteen
Tested the notebook, looks good and works fine. A few things still needed:

@rosteen
Copy link
Contributor Author

rosteen commented Apr 30, 2021

Updated, I think the last thing I'm likely to do is write some more detailed notes on the solutions to the exercises.

@rosteen
Copy link
Contributor Author

rosteen commented May 3, 2021

I think this is done! Just added detailed solutions to the exercises to the end of the _solutions notebook.

@camipacifici camipacifici merged commit d0db189 into spacetelescope:main May 4, 2021
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.

4 participants