Tickets/dm 6784 #1
Conversation
| if display: | ||
| ds9.mtv(exposure, frame=frame) | ||
| ds9.dot("x", center.getX() - exposure.getX0(), center.getY() - exposure.getY0(), frame=frame) | ||
| import pdb;pdb.set_trace() |
There was a problem hiding this comment.
Can we have this on two lines please? I don't think we have an exception for debugging. PEP-8 says "Compound statements (multiple statements on the same line) are generally discouraged.".
There was a problem hiding this comment.
I will leave it up to Paul to comment on himself, but I am betting that the pdb line should just be removed completely
There was a problem hiding this comment.
No, it provides the user an opportunity to inspect the image before it's replaced with something else. I'll make it two lines.
There was a problem hiding this comment.
I get the rational now, but it seems odd to use a debugging tool in this way, do we have precedence for doing this with other tools? Is there a way we could just use xpa commands to put up all the images into different ds9 frames so they all would be available?
There was a problem hiding this comment.
That code should be updated to use afwDisplay not ds9
There was a problem hiding this comment.
This has the advantage that you can test the display code is not crazy with the dummy device. The more code paths we can check the better. Of course, that means that linking debugging to display is not a good plan.
There was a problem hiding this comment.
I'm switching to lsst.afw.display as requested by RHL. I will continue to guard the display code with an if statement, because I don't want Jenkins trying to pop up ds9, failing and killing my build. Once RHL settles on how to select backends, we can revisit.
There was a problem hiding this comment.
Look at testDisplay.py in afw -- that allows you to select your own device on the command line but defaults to virtualDevice if you don't specify anything.
There was a problem hiding this comment.
A case could be made for that display selection code to be moved into a support package.
There was a problem hiding this comment.
I totally agree, and decided not to invent a mechanism when I wrote afw.display. I'll try to find time to write a c.l.o post to make progress. At that point the default can be virtualDevice.
I don't think we should mix up the choice of device with whether you want to display
things.
| pass | ||
|
|
||
|
|
||
| ApertureFluxConfig = makeConfigClass(lsst.meas.base.ApertureFluxControl) |
There was a problem hiding this comment.
Is there a specific reason that this is by itself at the top level?
There was a problem hiding this comment.
It's a full-fledged class, used within one of the other classes. I think defining it at the top level should be preferred to defining it within the ConvolvedFluxConfig class. It's not exported by default.
| @classmethod | ||
| def getExecutionOrder(cls): | ||
| return KronFluxPlugin.getExecutionOrder() + 0.1 # Should run after Kron because we need the radius | ||
| return cls.FLUX_ORDER |
There was a problem hiding this comment.
Is this leftover code cruft? I can't see how this return statement will ever be executed
There was a problem hiding this comment.
Yes, I'll kill it. Thanks!
| kronRadius = self.getKronRadius(measRecord) | ||
| except NoKronError: | ||
| kronRadius = 0.0 | ||
| return int(max(max(self.config.aperture.radii), self.config.kronRadiusForFlux*kronRadius) + 0.5) |
There was a problem hiding this comment.
Is the 0.5 just to get it to round up to the next int?
| convolved = image.Factory(bbox) | ||
| lsst.afw.math.convolve(convolved, subImage, kernel, lsst.afw.math.ConvolutionControl(True, True)) | ||
|
|
||
| # This is ugly, but necessary; should be resolved following RFC-217, DM-5503 |
| # Don't need the full set of apertures because the larger ones aren't affected by the convolution | ||
| self.aperture.radii = [3.3, 4.5, 6.0] | ||
|
|
||
| def getBaseNameForSeeing(self, seeing, name=PLUGIN_NAME): |
There was a problem hiding this comment.
I kind of find it weird to have all these getters in the config class, is there a reason they are in the Config and not in the plugin itself?
There was a problem hiding this comment.
is it simply to have access to the self.seeing?
There was a problem hiding this comment.
They don't need the plugin, but they do need the config: if you set the config, then you can determine the names. That suggests that they should be methods on the config. I guess they could also be free functions that take the config as a parameter, but I like putting them in the config because it makes them easier to call, and they really are functions of the config.
This allows us to write the measurement algorithm ("plugin") in python,
since all the heavy lifting is done using other packages. Deleted all
the C++ code and replaced with python implementation that works with the
modern measurement framework.
The test has been updated to allow use with pytest, and renamed following the desired naming scheme. Also added tests for the Kron (previously ignored).
PaulPrice
force-pushed
the
tickets/DM-6784
branch
from
d7be588
to
d429976
Compare
No description provided.