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
Tickets/DM-6824: Use meas.algorithms.astrometrySourceSelector in measOptimisticB #49
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comments. I think this is ok, but we should probably talk about what it would take to get astrometrySS to fit into where you're currently using the matchList.
@@ -3,25 +3,20 @@ | |||
from builtins import object | |||
import math | |||
|
|||
import numpy as np | |||
|
|||
import lsst.afw.table as afwTable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rerun your linter: looks like you no longer need the object or afwTable imports here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it still helps to run a linter rather then just relying on the highlighting in sublimetxt. It helps to pay attention huh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sublimetext is what caught it for me. I don't like committing a file with any warning dots at all. You're using SublimeLinter+flake8?
) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah the joy of deleting large blocks of code!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A yep. I noticed a missing double space in front of the main class so I have added that.
verbose=debug.verbose, | ||
) | ||
|
||
# cull non-good sources | ||
matches = [] | ||
# This is a hard coded version of the isGood flag from the old SourceInfo class that used to be | ||
# part of this class. This is done current as the API for sourceSelector does not currently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block should be lifted into a separate function, to make it easier to replace in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. I also moved this comment into the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also added another function for loading the schema keys since it wouldn't make sense to put it in the loop to test the isGood flag.
self.astrom = AstrometryTask(config=astrometryConfig, refObjLoader=refObjLoader) | ||
self.astrom.log.setLevel(logLevel) | ||
# Since our sourceSelector is a registry object we have to wait for it to be created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NP
Just pushed new version of matchOptimisticB.py with suggested changes. Adds two new functions for loading the source catalog schema and preforming the isGood test. Also removed afwTable from the imports and cleaned up some lint problems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment, but I'm still concerned about the fact that astrometrySS and matchLists don't play nice, thus you have to duplicate some source selection code in here.
self.saturatedKey = schema["base_PixelFlags_flag_saturated"].asKey() | ||
|
||
def _isGoodTest(self, source): | ||
# This is a hard coded version of the isGood flag from the old SourceInfo class that used to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a docstring, not comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this text to a docstring. We can discuss what changes we can make to BaseSourceSelector to make it play nice with match-lists and other "catalog like" objects next week if you would like.
matchOptimisticBTask now uses sourceSelectors from meas_algorithms instead of the SourceInfo class.
c8631e1
to
52730fd
Compare
Replaced SourceInfo in matchOptimisticB.py with a new sourceSelector object from meas_algorithms.