-
Notifications
You must be signed in to change notification settings - Fork 1
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
Adding MathPlag #1
Conversation
Lots of changes on behalf of checkstyle recommendations. Canonicalizer moved to another MathMLConverters project and added a few JavaDocs
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 overall. Just a few minor issues
} | ||
|
||
/** | ||
* Compare two CMMLInfo document against each other. The type defines how a Match will be displayed. |
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.
the comment is misleading
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.
Hm. Actually this method could use a overhaul.
} | ||
|
||
/** | ||
* Compare two CMMLInfo document against each other. The type defines how a Match will be displayed. |
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.
the comment is misleading
* | ||
* @param refDoc Reference CMMLInfo document | ||
* @param compDoc Comparison CMMLInfo document | ||
* @param type defines how a Match will be displayed (similar / identical) |
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 never manage to type string constants correctly. Isn't there a better way?
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.
Good point. Already used an enum but somehow never used it in the upper classes.
return result; | ||
} catch (Exception e) { | ||
// log and throw in this case | ||
logger.error("mathml comparison failed", e); |
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.
For debug and reply it would be super nice to have refMathML and compMathML available from the error message.
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.
Jep
* @param refDoc Reference CMMLInfo document | ||
* @param compDoc Comparison CMMLInfo document | ||
* @param onlyOperators find similarities only between operations, leafs are not checked | ||
* @return list of similarities, list can be empty but never null |
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.
"list can be empty but never null" isn't there a machine readable notnull annotation for 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.
Kind of, but it is not a fully supported feature in Java. Some code check libraries will notice certain annotation libraries.
} | ||
} else { | ||
// order insensitive | ||
List<MathNode> bChildren = new ArrayList<>(bTree.getChildren()); |
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 is somehow complex. Could you add a private subroutine for this type of comparison?
} | ||
|
||
/** | ||
* This method is still in testing. |
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.
ok what exactly does that mean? Can you add github issue with a description what needs to be done 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.
This method was fine in the mean time, but the other assessments method were not. I will open an issue.
import java.util.*; | ||
|
||
/** | ||
* This object represents the node of a mathematic expression tree (MET). |
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.
maybe use MEXT like we did in the paper?
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.
changed all occurrences
return null; | ||
} | ||
mathNode.setAttributes(node.getAttributes()); | ||
mathNode.setValue(node.getFirstChild() != null ? node.getFirstChild().getTextContent().trim() : node.getTextContent().trim()); |
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.
add a comment why this is necessary
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.
Good question ...
|
||
private void testSimilarities(String basicFilename) throws Exception { | ||
String mathmls = IOUtils.toString(this.getClass().getResourceAsStream(basicFilename + "_test.txt"), "UTF-8"); | ||
String[] split = mathmls.split("@@@"); // I use this as a simple split between two mathml's |
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.
why? I think it would be better to have a directory with individual files
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.
Jep
This pull requests adds the current MathPlag implementation. It will add classes, testcases and minor changes to the dependencies.