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

Flux calibration #287

Merged
merged 11 commits into from
Jan 29, 2025
Merged

Flux calibration #287

merged 11 commits into from
Jan 29, 2025

Conversation

JuergenSchreiber
Copy link
Contributor

@JuergenSchreiber JuergenSchreiber commented Jan 17, 2025

Describe your changes

  • add a new calibration class FluxcalFactor for a flux cal factor to convert electrons to erg/(scm^2AA) plus corresponding error as single values (like KGain) which is to be generated for each filter.
  • add functions to generate the calibration file.
  • add the conversion in l4_to_tda plus error propagation.

Type of change

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

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

#7 , #241

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
  • I have filled out the Unit Test Definition Table on confluence, as needed

@JuergenSchreiber
Copy link
Contributor Author

I made several assumptions:

  • The data is already background subtracted.
  • to generate the cal file I apply photutils tools. You can use either aperturePhotometry for circular apertures, where you can apply the fraction of the encircled energy in your aperture if you know the PSF or a 2D Gaussian extraction, just assuming that the PSF is Gaussian like.
  • Up to now I use Ra/Dec and WCS to determine a rough pixel position of the point source, but there might already be an exact astrometric pixel position determined before in the pipeline already in the header.
  • I had to update the photutils version in corgidrp requirements to >= 2.0.0 to get the Gaussian extraction

@maxwellmb maxwellmb self-requested a review January 24, 2025 19:21
@maxwellmb maxwellmb self-assigned this Jan 24, 2025
Copy link
Contributor

@maxwellmb maxwellmb left a comment

Choose a reason for hiding this comment

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

Overall this is looking good to me, please see the few comments.

I've also reviewed the associated UT Tests in the DRP Test Definition page. Were the tests for "Tests to determine apparent magnitude in obs band" ever approved by anyone? I think the PR has been merged, right?

corgidrp/data.py Outdated Show resolved Hide resolved
corgidrp/data.py Show resolved Hide resolved
corgidrp/data.py Show resolved Hide resolved
corgidrp/data.py Outdated Show resolved Hide resolved
corgidrp/fluxcal.py Outdated Show resolved Hide resolved
corgidrp/mocks.py Outdated Show resolved Hide resolved
corgidrp/mocks.py Outdated Show resolved Hide resolved
tests/test_fluxcal.py Outdated Show resolved Hide resolved
tests/test_fluxcal.py Outdated Show resolved Hide resolved
tests/test_fluxcal.py Outdated Show resolved Hide resolved
@JuergenSchreiber
Copy link
Contributor Author

Overall this is looking good to me, please see the few comments.

I've also reviewed the associated UT Tests in the DRP Test Definition page. Were the tests for "Tests to determine apparent magnitude in obs band" ever approved by anyone? I think the PR has been merged, right?

The answers are No, not yet approved, and yes, has been merged

@maxwellmb
Copy link
Contributor

maxwellmb commented Jan 27, 2025

Overall this is looking good to me, please see the few comments.
I've also reviewed the associated UT Tests in the DRP Test Definition page. Were the tests for "Tests to determine apparent magnitude in obs band" ever approved by anyone? I think the PR has been merged, right?

The answers are No, not yet approved, and yes, has been merged

Ok, great, thanks for letting me know. @semaphoreP did you look this over before accepting the PR, or should I still take a look?

@semaphoreP
Copy link
Contributor

Overall this is looking good to me, please see the few comments.
I've also reviewed the associated UT Tests in the DRP Test Definition page. Were the tests for "Tests to determine apparent magnitude in obs band" ever approved by anyone? I think the PR has been merged, right?

The answers are No, not yet approved, and yes, has been merged

Ok, great, thanks for letting me know. @semaphoreP did you look this over before accepting the PR, or should I still take a look?

I haven't looked at the UT Test Definitions for this test, so if you can review them, that'd be great.

@maxwellmb maxwellmb self-requested a review January 29, 2025 17:09
@maxwellmb maxwellmb merged commit b962fd7 into develop Jan 29, 2025
1 check passed
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