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-23624: Get MEDIAN_PER_ROW overscan correction moved to afw, or find numpy replacement #212

Merged
merged 3 commits into from Jun 6, 2022

Conversation

czwa
Copy link
Contributor

@czwa czwa commented May 17, 2022

Repurpose unused fitOverscanImage function for MEDIAN_PER_ROW.

This was already written as a pybind11 wrapped C++ function, but was unused in ip_isr. This changes the function to calculate the MEDIAN_PER_ROW method, improving processing time significantly (from ~.1s to ~1e-3s).

@czwa czwa changed the title Get MEDIAN_PER_ROW overscan correction moved to afw, or find numpy replacement DM-23624: Get MEDIAN_PER_ROW overscan correction moved to afw, or find numpy replacement May 17, 2022
@czwa czwa requested a review from natelust May 17, 2022 23:13
@mfisherlevine
Copy link
Contributor

That .1s is presumably per-amp, right? So this is actually more like a ~2s speedup for the full chip? If so, this is wonderful news! If not, I'm super sorry that I said this was a big deal if it really was only 0.1s total 😬

@czwa
Copy link
Contributor Author

czwa commented May 17, 2022

That .1s is presumably per-amp, right? So this is actually more like a ~2s speedup for the full chip? If so, this is wonderful news! If not, I'm super sorry that I said this was a big deal if it really was only 0.1s total grimacing

Yes, sorry. The numbers are per-amp.

@erykoff
Copy link
Contributor

erykoff commented May 18, 2022

This looks great! I would suggest renaming the PR subject for clarity about what's going on here (we have not moved it to afw, nor have we found a numpy replacement!).

python/lsst/ip/isr/overscan.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/overscan.py Outdated Show resolved Hide resolved
afw::math::StatisticsControl statControl;
statControl.setAndMask(overscan.getMask()->getPlaneBitMask(badPixelMask));
const int x0 = overscan.getX0();
const int y0 = overscan.getY0();
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest moving some things outside of the loop, so that objects won't keep being built on each iteration, and it will allow you to eliminate the branch in the loop (which branch prediction should be good about after a few iterations, but why not make it easier?

auto origin = geom::Point2I(x0 + x, y0);
geom::Extent2I shifter;
geom::Extent2I extents;
if (isTransposed) {
    shifter = geom::Extent2I(1, 0);
    extents = geom::Extent2I(1, height);
} else {
    shifter = geom::Extent2I(0,1);
    extents = geom::Extent2I(width, 1);
}

for (int x = 0; x < length; ++x) {
    MaskedImage mi = MaskedImage(overscan, geom::Box2I(origin, extents));
    values[x] = afw::math::makeStatistics(mi, afw::math::MEDIAN, statControl).getValue():
    origin.shift(shifter);
}

void fitOverscanImage(
std::shared_ptr<lsst::afw::math::Function1<FunctionT> > &overscanFunction,
template<typename ImagePixelT>
std::vector<double> fitOverscanImage(
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure no one is going to want this interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The one that's being replaced? I don't think anyone even noticed it was there until I found it last week.

@czwa czwa merged commit adc67c6 into main Jun 6, 2022
@czwa czwa deleted the tickets/DM-23624 branch June 6, 2022 19:33
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.

None yet

4 participants