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

AnalysisLevel does not own distinctive information #518

Open
odashi opened this issue Oct 2, 2022 · 4 comments
Open

AnalysisLevel does not own distinctive information #518

odashi opened this issue Oct 2, 2022 · 4 comments

Comments

@odashi
Copy link
Contributor

odashi commented Oct 2, 2022

Currently, every Processor distinguishes the process according to AnalysisLevel. This struct has name, features, and metric configs. Features and metric configs are specified by the processor itself, and they are only meaningful if the level is used with the same Processor. This means that the owner of these information is Processor, and thus AnalysisLevel is eventually only the key of these values, and not necessary to be formed as a struct. I think it is suitable to (1) remove this struct and (2) return just str as the key of the analysis level names.

RFC: @neubig

@neubig
Copy link
Contributor

neubig commented Oct 4, 2022

I think that's a reasonable alternative design and may be simpler.

My only concern (which I haven't thought about very carefully yet) is whether changing to this design would limit us in our ability to make features more customizable, which is one of our high priority tasks.

@odashi
Copy link
Contributor Author

odashi commented Oct 5, 2022

I think it is enough to provide a functionality to retrieve a list of features corresponding to given analysis level name.

class MyProcessor(Processor):
  def get_feature_set(name: str) -> List[Feature]:
    if name == "foo":
      return [...]
    elif name == "bar":
      return [...]

The returned features can be modified outside the processor.

@odashi
Copy link
Contributor Author

odashi commented Oct 5, 2022

I also think that the current AnalysisLevel can become an attribute of the Processor at initialization, or we can implement separate Processors for different AnalysisLevels, to avoid aditional control flow around AnalysisLevels.

@neubig
Copy link
Contributor

neubig commented Oct 5, 2022

The solution in the first of the two above comments seems reasonable to me for now! The latter ones seem like bigger changes that we'd have to think about more carefully

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

No branches or pull requests

2 participants