Changes in BratWriter #1170
Changes in BratWriter #1170
Conversation
It adds some parameters to allow: - indicate where the html template is located - define the sorting order of the annotations above each span - define the Feature to be shown for each annotation type
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Are you using Eclipse? If yes, could you please apply the code style available on our contribution website (http://dkpro.github.io/contributing/) to the changed portions of the code?
Some of the changes in this PR seem to particularly have a nicer visualization in mind, but it seems this happens at the sacrifice of being able to do a round trip UIMA<->brat. I think it is a good idea to improve the support for cleaner visualization, but I also believe this should be controlled via options, in particular if the changes create round-trip problems.
What do you think?
| { | ||
| this("T" + aId, aType, aBegin, aEnd, aText); | ||
| { | ||
| this("T" + String.format("%04d", aId), aType, aBegin, aEnd, aText); |
There was a problem hiding this comment.
@joancf You mentioned in the previous PR this change is done to permit sorting in the JS (btw. the change to the sorting in the JS code is not included in the present PR anymore). But using the padded numbers is kind of brittle as one needs to pre-suppose the number of annotations. How about instead changing the sorting code so that it strips the annotation type letter and then compares the rest of the ID numerically?
There was a problem hiding this comment.
Also we could have an ID of the form "layer-id" for sorting, so it will sort by layer (the layer can be a two digits padded number, more than 100 layers would be crazy)
for example 01-1 01-2 01-3 02-4 02-05 ....
| for (String currentType: spanTypes.keySet()){ | ||
| for (FeatureStructure fs : selectAll(aJCas)) { | ||
| String typeName=fs.getType().getName(); | ||
| String superType = fs.getCAS().getTypeSystem().getParent(fs.getType()).getName(); |
There was a problem hiding this comment.
This code seems to take into account only the type and the direct supertype of the FS. When taking into account the direct supertype, it seems sensible to actually check the entire inheritance hierarchy, no? org.apache.uima.cas.TypeSystem.subsumes(Type, Type) should be useful here.
Comment applies also to other changes further down.
There was a problem hiding this comment.
I just used the same way it was used somewhere else in the module. I understood that it was a method to specify the parent type instead of the all the subsumed types.
| relationFS.add(fs); | ||
| } | ||
| else if (hasNonPrimitiveFeatures(fs) && (fs instanceof AnnotationFS)) { |
There was a problem hiding this comment.
@joancf why did you entirely drop the support for annotations with multiple non-primitive features?
| BratEventAnnotation event = writeEventAnnotation(doc, (AnnotationFS) fs); | ||
| eventFS.put(event, fs); | ||
| } | ||
| else if (fs instanceof AnnotationFS) { |
There was a problem hiding this comment.
Why did you drop the fall-back handling for other types of annotations and the warning if an annotation could not be handled?
There was a problem hiding this comment.
Yes, in the same way as before: why should the component export elements the user hasn't ask for?
| @@ -562,7 +598,9 @@ private boolean isSlotFeature(FeatureStructure aFS, Feature aFeature) | |||
| private void writeRelationAnnotation(BratAnnotationDocument aDoc, FeatureStructure aFS) | |||
| { | |||
| RelationParam rel = parsedRelationTypes.get(aFS.getType().getName()); | |||
|
|
|||
| if (rel== null ) {// then is the parent type | |||
| rel=parsedRelationTypes.get(aFS.getCAS().getTypeSystem().getParent(aFS.getType()).getName()); | |||
There was a problem hiding this comment.
If we reach this function the type or his parent is in parsdRelationTypes
if the first one fails then is the parent.
There are soo many relation types that using the parent simplifies the definition.
|
|
||
| aDoc.addAnnotation(anno); | ||
|
|
||
| // writeAttributes(anno, aFS); |
There was a problem hiding this comment.
Why did you disable the ability to write attributes?
There was a problem hiding this comment.
They are not displayed, and I could not see any advantage of having them
|
Jenkins, can you test this please? |
|
@joancf Your changes rather target using the BratWriter to visualize annotations, not doing a lossless round trip CAS -> brat-format -> CAS, right? How about copying the BratWriter class into e.g. BratVisualizationWriter and focussing there on the kind of changes you made? That is probably easier for the moment than trying to handle the two use cases in the same component. |
refactoring of #1169
It adds some parameters to allow: