DM-9751: Verify the performance of new matchPessimisticB code on selected test fields #80
Conversation
| @@ -162,4 +162,71 @@ def _isUsable(self, source): | |||
| and not source.get(self.fluxFlagKey) \ | |||
| and self._goodSN(source) | |||
|
|
|||
|
|
|||
| class MatcherPessimisticSourceSelectorTask(MatcherSourceSelectorTask): | |||
There was a problem hiding this comment.
May I suggest PessimisticMatcherSourceSelectorTask as easier to read?
There was a problem hiding this comment.
I'd rather leave this as MatcherPessimistic... as it is a subclass of the matcher source selector. This source selector is somewhat temporary as the hope is to replace the current matcher with matcherPessimisticB. matcherSourceSelector will then become equivalent to matcherPessimisticSourceSelector and the later will be removed. I'm keeping the old one around to preserve most of the current matcher behavior before it is replaced.
| !Select sources that are useful for matching. | ||
|
|
||
| Good matching sources have high signal/noise, are non-blended. They need not | ||
| be PSF sources, just have reliable centroids. |
There was a problem hiding this comment.
Please rewrite the brief and full description to emphasize in what way this matcher differs from MatcherSourceSelectorTask.
Was it necessary to write a new selector instead of improving the old? Did you check to see if your updates harmed or helped WCS fitting using the old code? I can understand if you didn't want to go to that effort, but it seems quite likely your changes would help existing WCS fitting, and if so, it would be a win for code robustness and simplicity to improve the old task.
There was a problem hiding this comment.
Will do on the rewrite.
The plan currently is to replace the old matcher with the new matcherPessimsiticB after I run a large amount of data through the new matcher and confirm its performance. Hence why I don't not investigate the effect of the additional cuts on the old matcher. matcherPessimisticSourceSelector is essentially the combination of isGood and isUsable from the old SourceInfo class in matchOptimisticB. matcherSourceSelector is the isUsable part of the SourceInfo class.
|
|
||
| self.edgeKey = schema["base_PixelFlags_flag_edge"].asKey() | ||
| self.interpolatedCenterKey = schema["base_PixelFlags_flag_interpolatedCenter"].asKey() | ||
| self.saturatedKey = schema["base_PixelFlags_flag_saturated"].asKey() |
There was a problem hiding this comment.
I strongly suggest you rewrite this to call the parent class version and then add the extra stuff, in order to simplify the code and reduce code duplication. E.g.:
def _getSchemaKeys(self, schema):
MatcherSourceSelectorTask._getSchemaKeys(self, schema)
self.edgeKey = schema["base_PixelFlags_flag_edge"].asKey()
...
Please also use this technique for _isUsable_vector and try it for _isUsable (it might noticeably slow performance, but it might not).
| sourceSelectorRegistry.register("matcher", MatcherSourceSelectorTask) | ||
| sourceSelectorRegistry.register("matcherPessimistic", | ||
| MatcherPessimisticSourceSelectorTask) |
There was a problem hiding this comment.
If you rename the class (as I hope you will) please also adjust the name you use to register it.
|
I echo Russell's comment: I'd expect the star selector and matcher to be separate. If you need a new selector we can by all means add one, but it should not be tied to the choice of matcher (and hence the name should not include pessimistic). It would, of course, be better to just fix the issues with the old one. |
|
This is not explicitly a star selector. It only selects PARENT objects above a certain signal to noise threshold and a few checks on centroid quality flags. The difference between the matcher and matcherPessimistic selectors are the later also removes saturated, interpolated, and edge_key objects. They never explicitly select stars. They were originally part of the matchOptimisticB as the SourceInfo class. The selector isn't really specific to any matcher. I'm happy to change the name to something more generic for this class like for instance matcherNoSaturatedSourceSelctor. I currently have an epic to implement RFC-198 which will refactor all source selectors. At that point we can implement, for instance, a source selector that cuts on signal to noise and a set of badFlags if we don't like having a specific source selector for the matchers. Regardless the matcherPessimisticSourceSelector will go away once matchPessimisticB is adopted as the default matcher. I will put in an RFC to do this once I finish with epic DM-10399. matchPessimisticB is the improvement/bug fix/replacement for matchOptimisticB so I'm not looking to fix bugs with matchOptimisticB. |
| bad flags. It is a temporary addition designed preserve the source selction | ||
| used in matchOptimisticB. Once matchPessimisticB is addopted as the default | ||
| source selector the class will be removed and the saturated, interpoalted, and | ||
| edge_key flags will be added to the matcherSourceSelector class. |
There was a problem hiding this comment.
This is clear. I am not convinced others will be willing to abandon the old matcher as quickly as you would like (so I am not entirely happy with the class name for this star selector), but at least this matcher provides a useful separation.
Since replacing the old matcher with this one is technical debt, please file a ticket to replace the old selector (and matcher) and turn this into a TODO note that references the ticket number.
"interpoalted"
There was a problem hiding this comment.
I've added new text to this section. Could you please check to see that it is acceptable?
|
|
||
| TODO: Once DM-10399 is complete an RFC will be filed to make matchPessimisticB | ||
| the default matcher this class will replace matcherSourceSelector with this source | ||
| selector resulting in only one matcherSourceSeletor. |
There was a problem hiding this comment.
We can't get rid of the old source selector until we get rid of the old matcher. I suspect that will take a long time during which both matchers will be available. That sounds like two tickets: first make the new matcher the default, then months later (once folks sign of on it) delete the old matcher. These selectors won't be clean until we do the latter. DM-10399 is not the ticket for either of these. So I'm suggesting you file a new ticket and reference it here.
Better yet, just give this matcher a nice name that we can use for the long term and keep the name. That avoids technical debt and the need to reference any ticket.
There was a problem hiding this comment.
Would it be okay to make a 1 story point ticket titled "Merge matcherSourceSelector and matcherPessimsiticSourceSelector" and then explain the ticket is for after the new matcher is, hopefully, adopted?
There was a problem hiding this comment.
Created this new ticket DM-10800 in anticipation of the future adoption of matchPessimisticB.
There was a problem hiding this comment.
I still don't see how these two selectors can be merged; I think both have to live until the old matcher goes away. But thank you for creating a new ticket and let's go with that.
Cleaned up matcherPessimistic source selector. Added more comments and switched to interited methods. MatcherPessimisticSourceSelector has more comments stating what is different in it from MatcherPessimsiticSourceSelector. MatcherPessimisticSourceSelector now uses inherited methods base class instead of overwriting. Linted code. Fixed typos and added TODO mentioning DM-10399 Added mention of ticket DM-10800 DM-10800 merges the two matcher source selctors after matchPessimisticB is adopted as the default matcher.
No description provided.