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-12611: FrameDict(FrameSet const &) broken #34
Conversation
astshim.h did not include FrameDict.h
FrameDict(AstFrameSet*) was needlessly testing that the result was a FrameSet; it calls the FrameSet(AstFrameSet *) constructor, which performs the same test.
In C++ FrameDict(FrameSet const &) resulted in an unusable FrameDict, as AST had already deallocated the AST pointer. The pybind11 wrapper for that constructor did not show the problem, making the bug difficult to test in Python. Add a function that shows the bug in Python and call it in test_frameDict.py
FrameDict(FrameSet const &) constructed an unusable FrameDict because AST had already deallocated the raw pointer. Fix the constructor. The test added in the previous commit now passes.
8178593
to
e01670d
Compare
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.
Looks good. Couple of minor comments.
explicit FrameDict(FrameSet const &frameSet) : FrameSet(std::move(*frameSet.copy())), _domainIndexDict() { | ||
_domainIndexDict = _makeNewDict(*this); | ||
} | ||
|
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 constructor from an AstFrameSet*
still used for anything?
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.
Yes, it is used by Object::makeShim
indata = np.array([[1.1, 2.1, 3.1], [1.2, 2.2, 3.2]]) | ||
predictedOut = indata * self.zoom | ||
assert_allclose(frameDict.applyForward(indata), predictedOut) | ||
assert_allclose(frameDict.applyInverse(predictedOut), indata) | ||
|
||
frameDict2 = makeFrameDict(frameSet) | ||
self.assertEqual(frameDict2.getRefCount(), 1) |
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.
Out of curiosity, what was the failure? Was frameDict2.getRefCount()
0?
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.
Attempting to get the refcount fails.
No description provided.