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
DM-20566: Replace afwGeom with geom #46
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.
One question about a file, but otherwise this looks good. It would be nice to make a quick attempt at removing the broken code.
@@ -28,6 +28,7 @@ | |||
import lsst.afw.image as afwImage | |||
import lsst.afw.detection as afwDetection | |||
import lsst.afw.geom as afwGeom | |||
import lsst.geom as geom |
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.
You couldn't also remove the afwGeom
above?
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.
No, because makeSkyWcs
is used here.
17eac5f
to
8d327fd
Compare
flake8 tests not enabled because there are some fundamental issues in the code with variables being used that are not defined. See DM-4905, DM-5684
There are some places in here where variables are used that have not been defined. We don't seem to be using this code but it's easier to raise an exception rather than try to delete particular if-branches.
8d327fd
to
986a842
Compare
I've had a look at the 4 problematic areas. Fixes are not obvious so I've put in exceptions (they would throw any how) to be a bit more explicit about it. |
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.
Are you sure we shouldn't just remove those branches?
I'm not sure of anything, but since they are branches that people might encounter, even if we removed the code in the branch we should still retain the exception raising rather than ignoring the configuration option. I'm therefore inclined to leave it for now. |
There are some flake8 fixes as well but the package has some fundamental problems with variables being used that are not defined.