Dataset bug fix #157
Dataset bug fix #157
Conversation
We can't add this constraint. It doesn't make any sense.
Alternative fix for the code removed in 87de704.
| @@ -179,42 +175,6 @@ public String toString() { | |||
| super.visitGet(instruction); | |||
| } | |||
|
|
|||
| @Override | |||
| public void visitPropertyRead(AstPropertyRead instruction) { | |||
There was a problem hiding this comment.
Sorry, could I get a bit more info on why we are removing this handling? Just want to understand better
There was a problem hiding this comment.
Sure. This is my fault; the handling is wrong. What was happening below is that we were adding a constraint that basically said for x = obj.f then x := f, which completely ignores object ref. This was happening in a "for each element" traversal. I guess it solved some bug, but I found bugs, as you can imagine, it created. Adding the code above in the PythonTensorAnalysisEngine solved the same bug(s) but (I believe) in a correct way.
This is happening another instance of a weird test setup where some behavior works in this project but not in a client project. I believe the problem is related to #42. Our client uses Jython 3 but the Ariadne ML tests use just plain Jython. Since a core focus of Ariadne is to calculate the tensor shapes, we didn't want to switch to Jython 3 until #42 is fixed. Our client, however, is using Jython 3 for ML code because we are not yet using tensor shapes (just type inference for now). So, what happens is that it works here but not in our client, which winds up causing us to create multiple fixes for bugs :(.
I don't think #42 is a super involved fix, but it requires some investigation. We started ponder-lab#42 with some progress. I hope to get back to it soon.
There was a problem hiding this comment.
BTW, I think that Jython should be completely replaced by Jython 3 here at some point, meaning that we should end support for Python 2.
Now, we can track datasets without explicitly adding a constraint.