Ticket #833 (closed enhancement: fixed)

Opened 10 years ago

Last modified 10 years ago

Add a `normalize` flag to convolveLinear

Reported by: rhl Owned by: rowen
Priority: normal Milestone:
Component: afw Keywords:
Cc: becker@… Blocked By:
Blocking: #793 Project: LSST
Version Number: 3.3.16
How to repeat:

not applicable

Description

If we wish to convolve images with general PSFs (cf. #793) we need to support normalizing LinearCombinationKernels.

This seems reasonable to me. The function is currently called convolveLinear, but it would be natural to change it to convolve as the API will be identical to the convolve functions of other Kernel types.

The cost appears modest. The routine can calculate the sum of each of the linear components once per image (or store them in the Kernel class). It already knows the coefficients at every pixel, so the extra operations for a n-component NxN Kernel are n multiplies, n adds, and one divide. As we are already carrying out nN^2 multiplies and adds, this must be sub-dominant.

Change History

comment:1 Changed 10 years ago by rowen

  • Status changed from new to assigned

You are right. This makes a lot of sense.

comment:2 Changed 10 years ago by rowen

  • Owner changed from rowen to rhl
  • Status changed from assigned to needinfo

Is it sufficient to compute: normalized output pixel = sum (coeff_i * convolveAtPoint(basisKernel_i, image)) / sum (coeff_i)

In other words, is it sufficient to normalize the coefficients? This assumes that the collection of basis kernels is in some sense already normalized. If we cannot assume that, then how would you propose to normalize the basis kernels (some of which may well have an intrinsic sum of 0)?

comment:3 Changed 10 years ago by rowen

  • Owner changed from rhl to rowen
  • Status changed from needinfo to assigned

The requirement is that the kernel be normalized at each point. The kernel = sum(basisKernel_i * coeff_i). Each basis kernel basisKernel_i has a known sum of all pixels bkSum_i. So we just need to divide each convolved pixel by sum(bkSum_i * coeff_i) to perform normalization.

comment:4 Changed 10 years ago by rowen

  • Status changed from assigned to inTicketWork

comment:5 Changed 10 years ago by becker

  • Cc becker@… added

comment:6 Changed 10 years ago by becker

Another option is to create the basisKernel_i to have zero sum.

comment:7 Changed 10 years ago by rowen

  • Status changed from inTicketWork to assigned
  • Blocked By 806 added

I would prefer to merge the new code from ticket 806 to the trunk before starting work to reduce merge conflicts.

comment:8 Changed 10 years ago by rowen

  • Status changed from assigned to inTicketWork

comment:9 Changed 10 years ago by rowen

  • Owner changed from rowen to rhl
  • Status changed from inTicketWork to inStandardsReview

comment:10 Changed 10 years ago by rowen

comment:11 Changed 10 years ago by rowen

  • Blocked By 806 removed

(In #806) Removed blocking #833

comment:12 Changed 10 years ago by rowen

  • Owner changed from rhl to becker
  • Status changed from inStandardsReview to inTicketWork

Please review this (I had to set it to changes needed because the workflow doesn't permit directly reassigning a reviewer).

comment:13 Changed 10 years ago by becker

  • Owner changed from becker to rowen
  • Status changed from inTicketWork to assigned

The code looks correct, so I will sign off on it.

The only comment I will record for posterity is to consider / figure out when the trade-off between processing time and memory usage crosses over. Since this task could potentially be made faster with the accumulation into a temporary Image of

(**spFuncIter)(colPos, rowPos) * (*kSumIter)

inside the same loop that does the convolution, saving you (nCol*nRow) Function calls.

comment:14 Changed 10 years ago by rowen

  • Status changed from assigned to inTicketWork

This ticket just got reviewed and so I merged it to the trunk. However, the state somehow got back to "assigned", so I'm about to try to get it to the necessary state. There will thus be a lot of meaningless changes without comments.

comment:15 Changed 10 years ago by rowen

  • Owner changed from rowen to becker
  • Status changed from inTicketWork to inStandardsReview

comment:16 Changed 10 years ago by rowen

  • Owner changed from becker to rowen
  • Status changed from inStandardsReview to inTrunkMerge

comment:17 Changed 10 years ago by rowen

  • Owner changed from rowen to robyn
  • Status changed from inTrunkMerge to inQaReview

This has been merged. Time for the QA review, I guess. Are you willing to do this or reassign it to somebody, Robyn?

comment:18 Changed 10 years ago by robyn

Congratulations,

This will be the maiden voyage for automated standards checking.

I can provide separate three reports:

1) infractions of required LSST (must/shall) rules
2) infractions of recommended (should/may) LSST rules
3) potential bugs  (from Parasoft bug detective ruleset)

You will be overwhelmed if I send them all. I will start with the potential bug check. If there are no hits, then I'll move to the required LSST rules. I don't think we'll get further than that for most of DC3b. I don't want to alienate the developers.

And for full disclosure, less than 50% of the C++ Coding standards are currently implemented. And many rules are only partially implemented. For example, camel case naming only checks for first letter in correct case.

I will be providing a web pointer to the Parasoft Rule definitions which use derivative-LSST Rule numbers. Those pages are actually the test cases for the Rules Checker so will be updated over the next few weeks as I make the checks for a single rule also satisfy all other LSST rules (they get more wordy as I need to define, for example, copy constructors for every demo class, etc).

You will not be directly using the Standards Checker. We have a single license at the moment.

comment:19 Changed 10 years ago by robyn

Since the Bug Report results were so minimal, I prepared a report on the Required rules in the C++ Coding Standards document. In reviewing the output, I realize that some of the Required rules should be moved into the informational category. This will better highlight more serious errors. The report is coming your way under separate cover.

You may assist me by letting me know which errors are nuisance rather than requiring a thoughtful moment to determine if they should be ignored in the specific context.

Per your suggestion, I filtered the processing log to remove some of the extraneous debugging data. The parsing errors are more easily found now.

The Report and filtered Log are being sent via email.

comment:20 Changed 10 years ago by robyn

Date: Fri, 18 Sep 2009 13:44:26 -0700

Status from Russell on BugDetective? Report:

One problem with the report is that some code file paths are incomplete (truncated at the start), making it harder to find the source files the report is talking about. For example simpleFits.cc is listed as /afw/display/simpleFits.cc but the path is really python/lsst/afw/display/simpleFits.cc

As to content of this report:

python/lsst/afw/display/simpleFits.cc

  • The complaint about allocation is a false alarm.
  • I suspect that the "always evaulates to" complaints are due to template instantiation

srrc/math/SeparableKernel.cc

  • yes rowSum and colSum can be zero. This makes the kernel pixels inf, when perhaps NaN would be slightly more sensible. It is not worth special casing the code in my opinion.

Ditto for statistics.cc

comment:21 Changed 10 years ago by robyn

  • Status changed from inQaReview to closed
  • Resolution set to fixed

Russell,

As you have time, please fix up some of the infractions checked against the LSST Required Coding Standards. A fresh report will be generated the next time a Ticket against AFW is submitted.

Hopefully by the end of DC3b, all the serious infractions will be resolved. I'd like to move into DC4 with a relatively clean code base wrt serious infractions.

comment:22 Changed 10 years ago by robyn

  • Owner changed from robyn to rowen
Note: See TracTickets for help on using tickets.