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

Introduce JWST Boxcar Development Notebook from Ivo Busko #79

Merged
merged 5 commits into from
Feb 9, 2022

Conversation

duytnguyendtn
Copy link
Contributor

@duytnguyendtn duytnguyendtn commented Jan 28, 2022

Migrating @ibusko's notebook exploring implementing the JDAT notebook boxcar extraction algorithm into specreduce: https://github.com/ibusko/dat_pyinthesky/blob/spectral_extraction_tests/jdat_notebooks/spectral_extraction_tests/specreduce_extract_jwst.ipynb

This notebook also contains changes during my technical review

Co-authored-by: @ibusko
Co-authored-by: @ojustino

duytnguyendtn and others added 3 commits January 28, 2022 16:40
@duytnguyendtn duytnguyendtn marked this pull request as ready for review January 28, 2022 21:59
@duytnguyendtn
Copy link
Contributor Author

I can't request reviewers, but @tepickering should definitely review the algorithm we have here; also @ojustino should look at this just to rubber stamp the "final" version, since we dont' have @ibusko to confirm the small changes I made

@tepickering
Copy link
Contributor

i think the kosmos stuff can now be replaced with what's currently implemented in specreduce. at least for the simple, flat traces in that test image. however, it's also fine to leave for now to track the changes between what was done before and what can be done now.

@ojustino
Copy link
Contributor

ojustino commented Feb 1, 2022

Hi @duytnguyendtn, the notebook runs for me without issues. I agree with @tepickering that it would be good to switch the trace implentation from kosmos to specreduce, but that might be a task for later since getting the extraction algorithms into PRs seems to be the higher priority for now.

I have a couple of suggestions on the notebook itself:

  • The call to sys.path.insert() in cell 1 is no longer necessary, so I would remove it.
  • To avoid a warning, the second ax1.plot() call in cell 15 should have ':' as its second argument instead of 'k:'. color is specified in a later argument and having that curve be black doesn't make sense.

@duytnguyendtn
Copy link
Contributor Author

duytnguyendtn commented Feb 1, 2022

Good catch @ojustino ! I've updated the notebook.

leave for now to track the changes between what was done before and what can be done now.

This was my intention with introducing this notebook as it is @tepickering. I imagine that substitution can happen as a follow up to this effort?

@tepickering
Copy link
Contributor

that's what i was thinking. once i get the trace fitting implemented, i'll update this notebook as part of that effort.

@tepickering
Copy link
Contributor

shall i go forth and merge this?

@duytnguyendtn
Copy link
Contributor Author

@tepickering Yes please! I think we're good on our side to merge!

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