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

spectroscopy module #212

Merged
merged 37 commits into from
Oct 18, 2024
Merged

spectroscopy module #212

merged 37 commits into from
Oct 18, 2024

Conversation

neilzim
Copy link

@neilzim neilzim commented Sep 24, 2024

Describe your changes

  • New module containing step function prototypes for spectroscopic data calibration.
  • Test functions to verify the performance of prism-dispersed PSF centroids, dispersion profile fit, wavelength calibration, star spectrum registration, and line spread function fit.
  • New subdirectory for spectroscopy test data inputs under tests/test_data/spectroscopy/

Type of change

  • New feature (non-breaking change which adds functionality)

Reference any relevant issues (don't forget the #)

Checklist before requesting a review

  • I have linted my code
  • I have verified that all unit tests pass in a clean environment and added new unit tests, as needed
  • I have verified that all docstrings are properly formatted and added new documentation, as needed

Michele Woodland and others added 30 commits August 23, 2024 13:45
@semaphoreP semaphoreP changed the base branch from main to develop October 7, 2024 16:41
@semaphoreP
Copy link
Contributor

Hi @neilzim, the code here isn't in the model of step functions and data classes that we have for the rest of corgidrp. Is that something you have time to work on? If not, we need to find someone to make that interface before we can merge this code.

We also should put the FITS files used in the tests in Git LFS, instead of being tracked via regular git.

@neilzim
Copy link
Author

neilzim commented Oct 18, 2024

I will not have time to rework the functions + classes this fall.

@neilzim neilzim closed this Oct 18, 2024
@semaphoreP semaphoreP reopened this Oct 18, 2024
@semaphoreP semaphoreP changed the base branch from develop to spectroscopy October 18, 2024 21:43
@semaphoreP
Copy link
Contributor

Let's not close this PR. I'm going to merge your changes into a spectroscopy branch and we can see if we can get some help from the rest of the DRP team to refactor the code.

@semaphoreP
Copy link
Contributor

@neilzim, before I merge, can you update add the FITS files in this PR into the .gitattributes file in the base of the directory (following the syntax of the examples that are there (https://github.com/neilzim/corgidrp/blob/main/.gitattributes). That way, they get merged into this repo straight into git LFS. Hopefully this is quick and only takes a few minutes of your time.

@semaphoreP
Copy link
Contributor

If you give me write access to your fork of the repo, I can do it myself as well.

added spectroscopy test input files
@neilzim
Copy link
Author

neilzim commented Oct 18, 2024

Sure, I've just modified the gitattributes to include all the test inputs.

@semaphoreP
Copy link
Contributor

We need to filter=lfs diff=lfs merge=lfs -text appended to each line as well.

Copy link
Contributor

@semaphoreP semaphoreP left a comment

Choose a reason for hiding this comment

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

Ok for now, additional interface dev needed.

@semaphoreP semaphoreP merged commit ce20dec into roman-corgi:spectroscopy Oct 18, 2024
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.

2 participants