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

Update C code to reproduce Drizzlepac results #136

Closed
wants to merge 13 commits into from

Conversation

stsci-hack
Copy link

These changes correct differences between the C code used in this package with the original C code from drizzlepac which serves as the reference implementation for this algorithm. It insures that, for subarrays in particular, the distortion model gets applied to the correct input pixels as opposed to being off by (usually) 1.

@stsci-hack stsci-hack requested a review from a team as a code owner February 2, 2024 21:58
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.29%. Comparing base (f3f73e1) to head (a2079cc).
Report is 9 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #136   +/-   ##
=======================================
  Coverage   75.29%   75.29%           
=======================================
  Files           7        7           
  Lines         344      344           
=======================================
  Hits          259      259           
  Misses         85       85           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mcara
Copy link
Member

mcara commented Feb 3, 2024

That's strange... First, no unit tests are failing which indicates that we need unit tests to catch this bug. Second, do these changes affect only gaussian, tophat, and turbo kernels? In my tests (in the notebooks that I used to compare drizzle with drizzlepac.astrodrizzle) I was using the 'square' kernel. Is that one not affected by this PR?

@stsci-hack
Copy link
Author

That's strange... First, no unit tests are failing which indicates that we need unit tests to catch this bug. Second, do these changes affect only gaussian, tophat, and turbo kernels? In my tests (in the notebooks that I used to compare drizzle with drizzlepac.astrodrizzle) I was using the 'square' kernel. Is that one not affected by this PR?

My original testing running this modified code to create output images and comparing those drizzled images to ones created by drizzlepac using the same parameters (kernel, pixfrac, stepsize,...) showed that this code was able to reproduce the drizzlepac results. The comparisons were originally done for all kernels with this modified code. However, it turns out that when using a subarray instead of a full image uncovered differences in the drizzle code even with these corrections for subarrays. The tests I have just added not only demonstrate that, but provide a way to verify whether any further investigations and corrections to the drizzle code result in closer agreement with drizzlepac. It turns out that part of the differences stem from what look to be 'edge effects'. This makes some sense given how VERY DIFFERENTLY drizzle computes the bounding box for the inputs and outputs from the more empirical, on-the-fly checks that are done in drizzlepac. So, although these code changes may not fix everything as originally reported, hopefully these new tests will make it easy to track down the remaining differences in results.

@mcara
Copy link
Member

mcara commented Mar 11, 2024

My original question stemmed from discussions offline about discrepancies between drizzle and drizzlepac which you reported to be up to 40% 30% and I, in my own tests, if I recall correctly, could not detect differences larger than 0.4%. My tests were performed using the 'square' kernel. So, I thought this PR fixes or at least reduces these differences (which I have observed in the square kernel). However, I see that this PR does not touch square kernel code at all. That is what puzzled me.

Even so, I still wonder whether this "subarray-specific" bug that you are fixing here affects the square and the lanczos kernel??? For example, you are fixing the turbo kernel but not the square kernel which is closely related to the square kernel (the turbo). Are square and lanczos kernels not affected by this bug?

However, it turns out that when using a subarray instead of a full image uncovered differences in the drizzle code even with these corrections for subarrays. The tests I have just added not only demonstrate that, but provide a way to verify whether any further investigations and corrections to the drizzle code result in closer agreement with drizzlepac. It turns out that part of the differences stem from what look to be 'edge effects'. This makes some sense given how VERY DIFFERENTLY drizzle computes the bounding box for the inputs and outputs from the more empirical, on-the-fly checks that are done in drizzlepac.

I fail to see how 0.5 or 1 pixel shifts in subarrays can be related to edge effects. Even if edge effects exist, they should be limited to being near edges and not affect the middle of the image (away from borders) and thus there should not be any shifts in the middle.

@mcara
Copy link
Member

mcara commented Mar 11, 2024

Also, I cloned your branch and undid proposed code changes in the cdrizzlebox.c file only leaving the updated/new tests as is. I then run these new tests on the basically original cdrizzlebox.c and all tests passed. I am sorry, but I do not see how the tests in this PR could be useful if they are insensitive to the changes to cdrizzlebox.c proposed in this PR.

@stsci-hack
Copy link
Author

Given the difficulties in understanding the differences between drizzle and drizzlepac algorithms, I am withdrawing this PR at this time. Another PR will be needed later that more clearly identifies the cause of the differences and tracks changes in that code with more focused tests.

@stsci-hack stsci-hack closed this Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants