aphp / edsnlp

Modular, fast NLP framework, compatible with Pytorch and spaCy, offering tailored support for French clinical notes.
https://aphp.github.io/edsnlp/
BSD 3-Clause "New" or "Revised" License
115 stars 29 forks source link

Architecture choice on custom extensions #47

Closed bdura closed 1 year ago

bdura commented 2 years ago

Description

The way we've handled spaCy extensions in EDS-NLP has been erratic at best, with each pipeline declaring its own set of new extensions, cluttering spaCy's Underscore object.

For instance, the pipeline eds.dates, eds.measures and eds.emergency.priority all include a parsing component, and each pipeline saves its result in a different attribute.

We can clearly do better than that by adopting a more holistic and uniform approach.

Proposition

To start off the discussion, here are a few ideas/questions:

  1. Every extensions managed by edsnlp should probably land within a Obect._.edsnlp master attribute. This would avoid cluttering the extensions, and leave more room for the end users.
  2. We should regroup similar extensions as much as possible. In the example above, all three pipelines could write the parsing results to a unique key, for instance value or parsed.
  3. We could introduce a norm key, that contains the normalised variant for a given entity (eg stripped of pollution, accents, etc). A reasonable idea is to provide the text used for matching. The normalised variant should perhaps be computed on the fly, using a getter function ?
  4. To enable the use of getters and methods within the edsnlp extensions, we could use an Underscore object. Not sure if that is overkill and/or incurs significant added complexity.
bdura commented 2 years ago

@percevalw, @Thomzoy, @Aremaki, I'd love to get your thoughts on this!

percevalw commented 2 years ago

Great idea, I second you on this. Here are a few points of our IRL discussion

As most end users would either only use the existing attributes (negated, value, ...) and not write new ones, we should prefer a simpler notation as it is currently the case, under the base _ getter, but limiting the number of attributes. Therefore as suggested, we could use:

As the norm attribute is now strongly used throughout the lib as a depolluted ascii version of the text, changing it would probably mean refactoring most of the code. To generate semantically normalized text, we can use the __str__ and __repr__ methods on the generic value attribute.

Following your dates revamp, the _.value extension could inherit of a pydantic.BaseModel like:

class EDSValue(BaseModel):
  def __str__(self): ...
  def __repr__(self): ...
  ...

class Date(EDSValue):
  ...

class Measure(EDSValue):
  ...

class Concept(EDSValue):
  ...
bdura commented 2 years ago

Sounds good! :tada:

percevalw commented 2 years ago

As discussed, here is a potential solution that standardizes the current architecture for custom extensions @Thomzoy @aricohen93 Each component can create a Span extension named after the label of the entities it creates:

A specific ._.value extension is defined as an aggregator and retrieves the field associated with the label via a getter such that ent._.value == getattr(ent._, ent.label_). The str representation of ._.value could be the one displayed in the demonstrator.

This way, we can keep a consistent typing of each extension (tnm -> TNMScore, adicap -> AdicapCode, date -> Date, ...), while offering a unique entry point for some use cases via the value extension.

This does not prevent to define other extensions if needed, or to keep the old entity extensions and deprecate them in future versions.

percevalw commented 2 years ago

@Vincent-Maladiere

percevalw commented 1 year ago

Following the discussion with @Thomzoy, we carry on with the approach commented above:

For instance, the following (non-exhaustive) modifications should be made:

percevalw commented 1 year ago

These suggestions have been integrated in #213