kensho-technologies / bubs

Keras Implementation of Flair's Contextualized Embeddings
Apache License 2.0
26 stars 9 forks source link

Supports masking #20

Closed pinesnow72 closed 3 years ago

pinesnow72 commented 3 years ago

The ContextualizedEmbedding layer seems not to support masking. It does not set self.supports_masking = True and does not implement compute_mask()

Is it possible to add some code to support masking? For example, I try to implement compute_mask() as follows:

    def compute_mask(self, inputs, mask=None):
        (
            forward_input,
            backward_input,
            forward_index_input,
            backward_index_input,
            forward_mask_input,
            backward_mask_input,
        ) = inputs
        return [forward_mask_input, backward_mask_input]

Is this correct?

ydovzhenko commented 3 years ago

Interesting question! So the ContextualizedEmbeddingLayer itself does include a multiplication by the mask inputs here: https://github.com/kensho-technologies/bubs/blob/43d911d62de3af61ab629bebfa1c446e5bc0def9/bubs/embedding_layer.py#L190

But if you are planning to use masking further down the line in your own model, then yes - I can see why you'd need an additional function. I don't see any harm in adding a function like this.

Would you like to submit a PR with the function in your request? Only thing I would ask is a unit test for it (should be very quick). I'd happily review!

pinesnow72 commented 3 years ago

Interesting question! So the ContextualizedEmbeddingLayer itself does include a multiplication by the mask inputs here:

https://github.com/kensho-technologies/bubs/blob/43d911d62de3af61ab629bebfa1c446e5bc0def9/bubs/embedding_layer.py#L190

But if you are planning to use masking further down the line in your own model, then yes - I can see why you'd need an additional function. I don't see any harm in adding a function like this.

Would you like to submit a PR with the function in your request? Only thing I would ask is a unit test for it (should be very quick). I'd happily review!

Thank you for your reply. Actually, I tried to use this embedding for NER, which use BiLSTM-CRF, where need to use mask information. I succeeded in getting correct result for NER. Thanks again.