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
Point2I __repr__ reported wrong type. #288
Conversation
tests/test_box.py
Outdated
self.assertTrue(type(box.Extent()) is geom.Extent2I) | ||
box = geom.Box2D() | ||
self.assertTrue(type(box.Point()) is geom.Point2D) | ||
self.assertTrue(type(box.Extent()) is geom.Extent2D) |
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.
Might be slightly better to write this as e.g. assertIs(box.Point, geom.Point2I)
.
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.
So the problem is that box.Point
is some sort of pybind11 wrapper type. I could say self.assertIs(box.Point(), geom.Point2I)
. Is that acceptable?
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.
Can you elaborate? Type comparisons should Just Work for pybind11 wrappers.
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.
Is the point that we always prefer a specific test method over assertTrue
?
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.
@timj I think we should prefer specific tests if we can.
@kfindeisen This is what I'm seeing. I didn't know if I should expect this or not. The fact that the Point
on a Box2I
is a Point2D
is the bug I'm trying to fix in this ticket.
>>> box = afwGeom.Box2I()
>>> box.Point
<class 'lsst.afw.geom.coordinates.coordinates.Point2D'>
>>> type(box.Point)
<class 'pybind11_builtins.pybind11_type'>
>>> type(box.Point) is afwGeom.Point2D
False
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.
box.Point
should be a class object, so the last line makes sense. Is box.Point is afwGeom.Point2D
also false? What about box.Point == afwGeom.Point2D
?
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.
In python2
>>> box = afwGeom.Box2I()
>>> box.Point
<class 'lsst.afw.geom.coordinates.coordinates.Point2D'>
>>> type(box.Point)
<class 'pybind11_type'>
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.
Very interesting:
>>> box = afwGeom.Box2I()
>>> box.Point is afwGeom.Point2D
True
>>> box.Point == afwGeom.Point2D
True
Looks like I can say:
self.assertIs(box.Point, afwGeom.Point2D)
5623fd7
to
832dd19
Compare
No description provided.