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

better e2e test agreement with same file order, cosmic detection fix from latest II&T #215

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

kjl0025
Copy link
Contributor

@kjl0025 kjl0025 commented Oct 4, 2024

Describe your changes

I updated the trap-pump, synthesized master dark, traditional master dark, and k gain e2e tests to call the public-access repos from cgi_iit_drp to run the data in the script a la II&T, run the data in the script a la DRP, and then compare directly, all using the same file input order. I also updated the cosmic ray head flagging to only occur in the image area (in keeping with the latest from II&T). There is a pending PR on cgi_iit_drp with this cosmic flagging update.

These changes give exact agreement for k gain and trap-pump e2e tests and full-frame agreement to the < 1e-11 level for synthesized and traditional master dark e2e tests. The full-frame agreement is mainly due to the cosmic ray flagging fix. I would like to have a reviewer run these tests locally to ensure reproducibility, though. These e2e tests now require importing the II&T code (from cgi_iit_drp), but the casual user of the DRP shouldn't have those as mandatory libraries (i.e., I don't think they need to be listed in requirements.txt).

The other e2e tests appear to have already a high level of agreement between II&T and DRP.

Type of change

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

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

#207 , #204

Checklist before requesting a review

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

@JuergenSchreiber
Copy link
Contributor

I ran the file_order branch and indeed get no difference:
determined kgain: 9.484170888255141
determined read noise 139.82050344644298
difference to TVAC kgain: 0.0
difference to TVAC read noise: 0.0
error of kgain: [1.06210368]
error of readnoise: 21.151983297402197

But I wonder why we get such a different kgain value now, we had 8.8 in the first run?

reproduces 8.8145 e-/DN now b/c of file order
@kjl0025
Copy link
Contributor Author

kjl0025 commented Oct 9, 2024

I just pushed a change to the k gain e2e test. I figured Guillermo ran the data in the order according to the numbers at the end of each file name (i.e., the order in which the data was actually taken), and when I do this here, I get the 8.8145 e-/DN number from before. Since the script takes frame differences according to the order of the frames in the stacks, changing the order of files input can change the result some.

@JuergenSchreiber
Copy link
Contributor

OK, I also assumed to read the list according to the number in the file name and that they have a rising exposure time. Do we have a sorting problem of exposure times? I recognized that the files ....51836 to 51840 have a reduced exposure time of 1.9 sec, while the 5 files before have 19 sec exposure time. That means that the last 5 files would not fit into a scheme of rising exposure times with rising file number.

@kjl0025
Copy link
Contributor Author

kjl0025 commented Oct 10, 2024

No, I think we're good. And I don't think having increasing exposure time order of stacks matters. The DRP at least agrees with the ENG results. I think the result heavily depends on the order of files within a single-exposure-time sub-stack because there are only 5 per sub-stack for this particular dataset, and so there can be non-negligible variance in the frame differences for a single sub-stack, which can affect what k gain value is returned.

@semaphoreP
Copy link
Contributor

semaphoreP commented Oct 18, 2024

Hi @kjl0025, maybe we should start a requirements_e2etests.txt? It would be good to list the dependencies for testing in a systematic way. This way, we can direct testers (e.g., CTC) to run pip install -r requirements_e2etests.txt corgidrp to install the necessary test dependencies. We would probably need to include each of the 3 packages in the repo as separate lines in the requirements file.

I will try to install cgi_iit_drp and run the tests now.

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