-
Notifications
You must be signed in to change notification settings - Fork 36
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
Comments
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. |
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. |
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. |
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 |
Currently, every
Processor
distinguishes the process according toAnalysisLevel
. 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 isProcessor
, and thusAnalysisLevel
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 juststr
as the key of the analysis level names.RFC: @neubig
The text was updated successfully, but these errors were encountered: