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

Bug: `explain` type is incorrect in `History` factory #116

Closed clementjumel closed 2 years ago

clementjumel commented 2 years ago

Hi! I find your work with edsnlp really cool, and I'm working on integrating it in one of our tools at Arkhn πŸ™‚ I found a very little mistake which prevents us from trying the History component at all and which should be very easy to fix!

Description

In the History class, you use an attribute explain, which is a boolean, however in the corresponding factory, you made it a string (right here).

As a consequence, spaCy converts the parameter to a string, so the "explain mode" is always on, which is an issue when we try to serialize the documents (e.g. with doc.to_bytes()) as the history cues are not serializable. 😬

How to reproduce the bug

import spacy

nlp = spacy.blank("fr")
nlp.add_pipe("eds.history")

name, component = nlp.pipeline[-1]
print(name)
print(component.explain)
print(type(component.explain))
print(bool(component.explain))

which gives:

eds.history
False
<class 'str'>
True

the last line exhibits that the explain feature here is always activated πŸ˜•

Your Environment

percevalw commented 2 years ago

Hi @clementjumel, thanks for using the library and letting us know about this issue! Would you like to submit a PR to fix this?

clementjumel commented 2 years ago

Hi @clementjumel, thanks for using the library and letting us know about this issue! Would you like to submit a PR to fix this?

Sure, here it goes: #117 πŸ™‚

For some reason, locally, not all tests were passing before & after the change, even following the installation described here. 😬 Besides, given the nature of the fix, I don't think it's worth the implementation of tests for the fix!