search

GateNLP / gate-core

The GATE Embedded core API and GATE Developer application
GNU Lesser General Public License v3.0
75 stars 29 forks source link

Is FeatureMap "live"? #71

Closed drj11 closed 5 years ago

drj11 commented 5 years ago

getFeatures() on the gate.util.FeatureBearer interface does not say whether the returned FeatureMap is live or not. By live I mean that updates to the FeatureMap actually change the Map on whatever object you invoked getFeatures on. The alternative would be that getFeatures() returns a clone of a FeatureMap and it is useless trying to change it.

The two behaviours are quite different, and the FeatureBearer interface should document which it is expecting.

ianroberts commented 5 years ago

In general yes, calling getFeatures gives you a reference to the Map that you can put new values into and they will be seen by anyone else who has a reference to the same map. In general it's a bad idea to share the same FeatureMap object between different annotations but I know there are cases in real apps where we do this (whether intentionally or otherwise).

ianroberts commented 5 years ago

Many common use cases in GATE rely on being able to add new features to an existing annotation (e.g. the POS tagger adds a category feature to Token annotations).

I guess this was probably one of those cases where the normal Java behaviour (returning a reference to the existing object) is assumed if it doesn't state otherwise. The difference between "returns this object's FeatureMap" vs "returns a copy of this object's FeatureMap"

drj11 commented 5 years ago

So I agree with all of this, but this is an Interface; an implementation could choose to do anything. Where the interface prefers or requires a particular behaviour (returning a reference to, not a reference to a copy of) then we could add that to the documentation.

Concretely how do I know that the following code (from my Module 8 exercise 2 from the GATE training) works:

    FeatureMap feats = doc.getFeatures();
    feats.put("date", new Date());

and that I don't have to explicitly set the entire FeatureMap with:

    FeatureMap feats = doc.getFeatures();
    feats.put("date", new Date());
    doc.setFeatures(feats)

Now that I've found the documentation: https://github.com/GateNLP/gate-core/blob/8b89090f10791274b7a2b784503288ed90a64ce2/src/main/java/gate/util/FeatureBearer.java#L24 Shall I submit a PR?

greenwoodma commented 5 years ago

If you think it needs further clarification then feel free to simply fix it (don't think it needs a pull request but up to you).

As @ianroberts said I don't think it's an issue as it follows general Java philosophy of returning a reference. If it was returning a copy then the documentation would explicitly mention that, because it wouldn't be the normal Java behaviour. As you say though it's on an interface so in theory an implementation could do something weird; mind you adding documentation to the interface doesn't stop that happening anyway.