search

roman-corgi / corgidrp

Data Reduction Pipeline for the Roman Coronagraph Instrument
BSD 3-Clause "New" or "Revised" License
5 stars 4 forks source link

Updated docstrings for k-gain and non-linearity calibration #216

Closed hsergi closed 3 weeks ago

hsergi commented 1 month ago

Describe your changes

As part of the effort to review the doc in the calibration functions before moving to e2e simulations, I have updated docstrings for calibrate_kgain.py and calibrate_nonlin.py

Type of change

I have updated the main docstrings of calibrate_kgain.py and calibrate_nonlin.py describing what the data structure is in a common fashion for both calibration functions. The same information can be found on Confluence (link). It has been run thru Guillermo Gonzalez.

I also fixed a condition on the number of frames used to calibrate non-linearity in calibrate_nonlin.py

I have checked that e2e tests pass the same way as before the docstring changes.

Checklist before requesting a review

semaphoreP commented 1 month ago

Should we wait until we have a discussion on new header formats before finalizing the updated docstring @hsergi?

hsergi commented 1 month ago

Sounds good. Marie asked me to work&submit this review that is compatible with v1.0. It clarifies a bit the structure of the cal data. We can wait, especially once we know how much work is required to update the use of the keywords.

semaphoreP commented 1 month ago

If it's for clarifying v1.0, this PR should be submitted to main.

Given this may change after our meeting this week, I'd like to hold off on PR.

hsergi commented 1 month ago

Sounds good. Although the main purpose was to clarify the docstrings, notice that "I also fixed a condition on the number of frames used to calibrate non-linearity in calibrate_nonlin.py", so there's a bug too. That's why I chose develop instead.

semaphoreP commented 3 weeks ago

Given the conversation from our meeting, shall we just PR this docstring update in? I think it's still consistent with the plan moving forward

semaphoreP commented 3 weeks ago

Bug fix for calibrate nonlinearity should be propogated to v1.1 also right? If that's the case, I'd favor merging into main