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

DM-44169: Fix compensated gaussian measurement error raising code #273

Merged
merged 2 commits into from
May 3, 2024

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented May 2, 2024

No description provided.

@laurenam
Copy link
Contributor

laurenam commented May 3, 2024

It seems you should refactor your commits...I don't think leaving the first one is a good idea (i.e. you now know MeasurementError should not be used, so history would prefer to forget!) Also, the last commit now combines the code changes with the unittest updates. I'd be inclined to keep them as separate commits (and just using the final form here).

@erykoff
Copy link
Contributor Author

erykoff commented May 3, 2024

Refactored the commits!

# failure flags
failure_flag = flagDefs.add(f"{width}_flag", "Compensated Gaussian measurement failed")
oob_flag = flagDefs.add(f"{width}_flag_bounds", "Compensated Gaussian out-of-bounds")

Copy link
Contributor

Choose a reason for hiding this comment

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

I think width evaluates to to a number (integer). Do you think it will be apparent "enough" that this number in the flag name is the kernel width? And will the casual user be expected to translate bounds to outOfBounds internally (probably yes to both, but my ~10pm brain didn't jump there right away)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the name of the measurement ... base_compensatedGaussian_5 or whatever. It's what we decided on ages ago, and it may not even matter since the compensated gaussians are not working well as a calibration flux measurement. 😞
As for the bounds, this is also the name from before, except done correctly. I don't think this is going to be used for anything anyway.

y_slice = slice(y_floor - rad, y_floor + rad + 1, 1)
x_slice = slice(x_floor - rad, x_floor + rad + 1, 1)
y_mean = y - y_floor + rad
x_mean = x - x_floor + rad

sub_im = exposure.image.array[y_slice, x_slice]
sub_var = exposure.variance.array[y_slice, x_slice]
Copy link
Contributor

Choose a reason for hiding this comment

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

You could technically move setting sub_var to after the size checking (but readability may win out to leaving here...up to you).

@erykoff erykoff merged commit 62aa8f1 into main May 3, 2024
2 checks passed
@erykoff erykoff deleted the tickets/DM-44169 branch May 3, 2024 15:27
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.

2 participants