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

Add support for co-adding with squared weights #163

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

Conversation

mcara
Copy link
Member

@mcara mcara commented Dec 7, 2024

This PR adds support for resampling and adding resampled (error, variance) images with squared weights. This is to facilitate standard error propagation.

@mcara mcara self-assigned this Dec 7, 2024
@mcara mcara requested a review from a team as a code owner December 7, 2024 07:28
@mcara mcara requested review from nden and melanieclarke December 7, 2024 07:28
Copy link

codecov bot commented Dec 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.66%. Comparing base (8978081) to head (a4f6065).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #163      +/-   ##
==========================================
+ Coverage   97.27%   99.66%   +2.39%     
==========================================
  Files           3        2       -1     
  Lines         220      300      +80     
==========================================
+ Hits          214      299      +85     
+ Misses          6        1       -5     

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

@mcara mcara requested review from schlafly and drlaw1558 December 7, 2024 07:48
@mcara mcara force-pushed the add-resampling-with-square-weights branch from 5282430 to cf658bf Compare December 7, 2024 07:55
Copy link

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

A couple questions and suggestions below, but in particular: the weights and area overlaps look like they are squared in the right places, but I think the pixel scale also needs to be squared before applying it to the squared data arrays.

drizzle/resample.py Outdated Show resolved Hide resolved
drizzle/resample.py Outdated Show resolved Hide resolved
drizzle/resample.py Outdated Show resolved Hide resolved
drizzle/resample.py Outdated Show resolved Hide resolved
src/cdrizzleapi.c Outdated Show resolved Hide resolved
src/cdrizzlebox.c Outdated Show resolved Hide resolved
src/cdrizzlebox.c Outdated Show resolved Hide resolved
src/cdrizzlebox.c Outdated Show resolved Hide resolved
src/cdrizzlebox.c Outdated Show resolved Hide resolved
src/cdrizzlebox.c Outdated Show resolved Hide resolved
@mcara mcara force-pushed the add-resampling-with-square-weights branch 3 times, most recently from 568b72e to b2c4529 Compare December 10, 2024 00:05
@mcara mcara force-pushed the add-resampling-with-square-weights branch from b2c4529 to 8690bee Compare December 10, 2024 00:18
@mcara mcara force-pushed the add-resampling-with-square-weights branch from bef3e11 to 918ca73 Compare December 10, 2024 08:45
Copy link

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

Everything looks good now, from my perspective!

Copy link
Collaborator

@kmacdonald-stsci kmacdonald-stsci left a comment

Choose a reason for hiding this comment

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

Minor comments, but otherwise looks good.


self._out_img2 = []

if (isinstance(self._fillval2, str) and
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend putting the and at the beginning of the next line, rather than at the end of a line, to make it easier to see.

del arr

def _alloc_output_arrays2_add(self, ninputs2=None):
if (isinstance(self._fillval2, str) and
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend putting the and at the beginning of the next line to make it more visible.

dtype=np.float32
)

if ((ninputs2 is not None and ninputs2 != nimg2) or
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend moving the or to the beginning of the next line to make it more visible.

@@ -48,16 +48,160 @@ scale_image(PyArrayObject *image, double scale_factor) {
* Top level function for drizzling, interfaces with python code
*/

static int
process_array_list(PyObject *list, integer_t *nx, integer_t *ny,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment to document what this function does, as well as what the function parameters are.

Py_XDECREF(arr_list[i]);
}
free(arr_list);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs a return value.

free(arr_list);
}
}

static PyObject *
tdriz(PyObject *obj UNUSED_PARAM, PyObject *args, PyObject *keywords) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function roughly 400 lines long and should be broken down into smaller sub-functions.

@@ -228,47 +536,38 @@ tdriz(PyObject *obj UNUSED_PARAM, PyObject *args, PyObject *keywords) {
p.exposure_time = expin;
p.weight_scale = wtscl;
p.fill_value = fill_value;
p.fill_value2 = fill_value2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

One letter variable names should be avoided for things other than indices. I recommend changing p to a more descriptive variable name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants