Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor improvements to the Embedder #35

Merged
merged 6 commits into from Feb 18, 2022
Merged

Conversation

LeonardoEmili
Copy link
Contributor

@LeonardoEmili LeonardoEmili commented Feb 17, 2022

The following changes have been applied:

  • Add support to average the last four hidden layers of the transformer model
  • Add a shape property to the TransformersEmbedderOutput class (referenced in the README)
  • Add option to specify which hidden states to use for pooling
  • Update the docs accordingly
  • Run black in compliance with the project specifications
  • Fix documentation issues

@Riccorl Riccorl self-assigned this Feb 17, 2022
@Riccorl
Copy link
Owner

Riccorl commented Feb 17, 2022

I suggest to change

def __init__(
        self,
        model: Union[str, tr.PreTrainedModel],
        return_words: bool = True,
        pooling_strategy: str = "last",
        output_layers: List[int] = [-4, -3, -2, -1],
        fine_tune: bool = True,
        return_all: bool = False,
    ) -> None:
        super().__init__()

to

def __init__(
        self,
        model: Union[str, tr.PreTrainedModel],
        return_words: bool = True,
        pooling_strategy: str = "last",
        output_layers: List[int] = None,
        fine_tune: bool = True,
        return_all: bool = False,
    ) -> None:
        super().__init__()
        if output_layers is None:
             output_layers  = [-4, -3, -2, -1]

to pass the checks.

@LeonardoEmili
Copy link
Contributor Author

LeonardoEmili commented Feb 17, 2022

I suggest to change

def __init__(
        self,
        model: Union[str, tr.PreTrainedModel],
        return_words: bool = True,
        pooling_strategy: str = "last",
        output_layers: List[int] = [-4, -3, -2, -1],
        fine_tune: bool = True,
        return_all: bool = False,
    ) -> None:
        super().__init__()

to

def __init__(
        self,
        model: Union[str, tr.PreTrainedModel],
        return_words: bool = True,
        pooling_strategy: str = "last",
        output_layers: List[int] = None,
        fine_tune: bool = True,
        return_all: bool = False,
    ) -> None:
        super().__init__()
        if output_layers is None:
             output_layers  = [-4, -3, -2, -1]

to pass the checks.

Wouldn't it mean loosing default value suggestions on the IDE? What about passing the layers as a tuple instead of a list?

@Riccorl
Copy link
Owner

Riccorl commented Feb 18, 2022

Let's try with the tuple first.

@Riccorl Riccorl merged commit 88c6d52 into Riccorl:main Feb 18, 2022
@Riccorl Riccorl self-requested a review February 18, 2022 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants