neulab / ExplainaBoard

Interpretable Evaluation for AI Systems
MIT License
359 stars 36 forks source link

Remove unnecessary Optional #514

Closed odashi closed 1 year ago

odashi commented 1 year ago

This change removes several Optional and | None annotations from list and dict types that can represent nothing by the empty collection. This change suppresses some unwrap invocations from the codebase.

This change also involves several typing fixes.

neubig commented 1 year ago

Thank you! I think this is a reasonable change, but I do feel that the semantics of none and the empty list or dictionary are a little bit different in some of these cases. None generally refers to " has not been calculated yet " we're an empty list or dictionary more indicates that it has been calculated but is empty. Do you think it would be better to remove this implicit distinction? Of course we could make it explicit where necessary, but that adds a bit of complexity to the code

odashi commented 1 year ago

None generally refers to " has not been calculated yet " we're an empty list or dictionary more indicates that it has been calculated but is empty.

I'm not sure this is really a general policy in Python, but I don't recommend giving any other meanings than "never" for None. It is actually confusing because the value does not say anything about the underlying semantics, and we need careful documentation about it.

odashi commented 1 year ago

I think SysOutputInfo (for example) has too many information that are not necessary to be co-existed. Giving/passing this structure seems to confuse overall behavior of the library.

neubig commented 1 year ago

OK, sounds good, let's go ahead and remove the Nones then. I'll do the review soon.