Tickets/DM-8645: Implement new matcherSourceSelector object for use in matchOptimisticB code #56
Conversation
|
Where's the unittest? It doesn't appear to have come along with these changes. |
There was a problem hiding this comment.
Several comments about cleaning up the docstrings, a concern about not checking the "bad" flags, and you need to add the unittests. Also, please rebase flatten several of these commits together.
Separately, I think we can probably make astrometrySourceSelector a child class of matcherSourceSelector (they share quite a few methods).
| """ | ||
| !Select sources that are useful for astrometry. | ||
|
|
||
| Good astrometry sources have high signal/noise, are non-blended, and |
There was a problem hiding this comment.
You need to change this docstring, since it's still verbatim the one from astrometrySourceSelector. Might be worth being more explicit about the differences between this and astrometrySS (in both of their docstrings). Also please take out all the preceding ! and reformat things as numpydoc, instead of doxygen.
There was a problem hiding this comment.
As stated in the other comment I'm not aware of the new docstring standard. This is copied verbatim from astrometrySourceSelector. I can re-do it if absolutely necessary. I have already edited the doc strings and will push the changes shortly.
|
|
||
| def selectSources(self, sourceCat, matches=None): | ||
| """ | ||
| !Return a catalog of sources: a subset of sourceCat. |
There was a problem hiding this comment.
Please reformat this as numpydoc.
There was a problem hiding this comment.
This was was as is and is straight from the astrometrySourceSelector. Not sure how to make it a numpydoc and if we would need to change the base class as well.
There was a problem hiding this comment.
Ok, I guess we'll hold off on reformatting as numpydoc (I did astrometrySS before that RFC had been fully settled). New python code is allowed (an I believe recommended) to use numpydoc instead of doxygen.
| self._getSchemaKeys(sourceCat.schema) | ||
|
|
||
| if sourceCat.isContiguous(): | ||
| good = self._isUsable_vector(sourceCat) |
There was a problem hiding this comment.
I think you still want to remove things with the "bad" flag here. Do you really want cosmics, interpolated pixels and stuff on the edge of a chip?
There was a problem hiding this comment.
I do want interpolated pixels and edge of chip things. Which specific bad flag tests for cosmics? I can add that if we want. I'm mostly just following the tests verbatim from the old SourceInfo class in matchOptimisticB.py which does not test for those things.
There was a problem hiding this comment.
The base class I think already defines all the ones you want in the badFlags config parameter. Cosmics are crCenter, I think.
I really do think you should consider what the "correct" source selection criteria are, instead of just reproducing SourceInfo: there's really no guarantee it was fully thought out.
There was a problem hiding this comment.
Isn't this more of a new ticket then? As in implementing the source selector framework first and then testing a new selection criteria later? Wouldn't that make it easier to track the changes to the code? Or am I over thinking this?
There was a problem hiding this comment.
That's a fair point. File the ticket(s) about figuring out what "correct" source selection for matcherSS would mean, and link them from 8645.
There was a problem hiding this comment.
This issue has been filed as a ticket here: https://jira.lsstcorp.org/browse/DM-8987
|
Added unittest for matcherSourceSelector to the ticket after it was erroneously not added. Have not yet modified the doc strings as these are the same as currently implemented in the base source selector and astrometrySourceSelector. Perhaps we should open a new ticket to modify all of these? Also I have not yet added in isBad flags as none of these flags were used in the original SourceInfo for matchOptimisticB which matcherSourceSelector is based on. Can add these in if completely necessary. |
| import lsst.utils.tests | ||
|
|
||
|
|
||
| badFlags = [ |
There was a problem hiding this comment.
Should have a comment here describing what these flags are for.
There was a problem hiding this comment.
Hey here's the current text I've written. Let me know if this is what you want and I'll commit it.
"
badFlags are flags that, if the bad flag is set the objecct should be
imediately thrown out. This is as opposed to combinations of flags
signifying good or bad data. Such flags should then be a part of the
isGood test
"
There was a problem hiding this comment.
Such bold, much wow...
That's probably fine? Though those bad flags aren't used by matcherSS right now, right?
There was a problem hiding this comment.
That is correct. Again, this is so that it is identical to the SourceInfo test.
There was a problem hiding this comment.
I forget what we decided, was I going to put this test in to matcherSourceSelector or into BaseSourceSelector?
There was a problem hiding this comment.
This block is being removed from testMatcherSS, and there is no "testBaseSS" (can't be: its abstract). So, the above text could go into testAstrometrySS.
| schema.addField(flag, type="Flag") | ||
|
|
||
| self.src = afwTable.SourceCatalog(schema) | ||
| self.sourceSelector = sourceSelector.sourceSelectorRegistry['matcher']() |
There was a problem hiding this comment.
I think here you can just construct the matcherSourceSelector directly (unless I'm missing something). On the other hand, this does test that you correctly put it in the registry, so...
There was a problem hiding this comment.
I think we can leave it as is if that a necessary test, that is checking to see that it is properly registered. Again, if we change this we will should also modify the tests for astrometrySourceSelector as it is doing a similar thing.
| self.assertIn(x, result.sourceCat['id']) | ||
|
|
||
| def testSelectSources_bad(self): | ||
| for i, flag in enumerate(badFlags): |
There was a problem hiding this comment.
Doesn't matcherSourceSelector ignore badFlags? So how does this test pass?
There was a problem hiding this comment.
Some of the badFlags are redundant with tests preformed in the isGood test. This is true of the astrometrySelector as well. This is why the is badFlag test still passes.
There was a problem hiding this comment.
Its not redundancy that I'm worried about: if the flag is set and the test expects a failure, how can this test pass?
There was a problem hiding this comment.
Because it will still fail isGood which it is awesome testing.
There was a problem hiding this comment.
Since we talked about it: you can delete the _bad test, since you're not using bad flags at all. The two slot flags are really part of testing the negation of isGood (I realized now that testAstrometrySS does this in an unfortunate way). Once you figure out whether matcherSS needs bad flags, you can add a test back in.
There was a problem hiding this comment.
Also I believe we wanted to remove this test. Is this the case?
There was a problem hiding this comment.
Yes: see my comment just above.
matcherSourceSelector.py impliments the isUsable criteria from SourceInfo in matchOptimistB.py. It is mostly a clone of astrometrySourceSelector.py with less stringent tests.
morriscb
force-pushed
the
tickets/DM-8645
branch
from
45ab745
to
81a41ee
Compare
morriscb
changed the title
morriscb
changed the title
morriscb
changed the title
Implemented new matcherSourceSelctor for use in meas_astrom with matchOptimisticB.