This repository was archived by the owner on Mar 16, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 0
This repository was archived by the owner on Mar 16, 2022. It is now read-only.
My feedback and thoughts #1
Copy link
Copy link
Open
Labels
discussion-neededOpen for discussionOpen for discussion
Description
Overall, it looks good. Introducing the dedicated class and adding preprocessing transform feels a major improvement.
The following is my thoughts reading through the README and examples.
Major points
- Do we want to have parent the
Weightsclass? I think the proposition can also be anidiom of best practicefor the flexibility, which different project can implement without having a parent class.check_typemethod is nice but it is simple enough (isinstance) to perform the check. Also what do you think of allowing the customize the behavior with duck-typing, say, in-house models?- The treatment of
latest: boolfeels a bit tedious for maintenance. When I add a latest model, I would like to be care-free about the previously-latest model. Since only one of them is supposed to be `True, how about simply making it a hard-coded class attribute?
- The idea of
latest: boolmight not synthesize well with audio. Say, I have a model architecture for Speech Recognition, I can have multiple of SOTA/latest models because of language / expected environments. (Note it is common to train and deploy multiple models with the same architecture, so that one is optimized for meeting room dictation, another is optimized for phone-conversation etc...) - What if a model needs post-processing? Is the current framework extendable? For example, when the preprocessing is FFT, then post-processing can be the inverse of the specific FFT applied during the preprocessing.
Minor points
- The naming
Weightsfeels slightly off becausetransformsnot only contains weights but also defines the preprocessing operations as well. state_dictmethod should accept**kwargs, which will be passed toload_state_dict_from_url, so that the downloading process can be customized. (i.e. download location and such)- I found it so hard to make Sphinx work well with Enum. Did you try?
- Currently,
partialis used for the definition oftransforms. I think this is good, because it will not instantiate an object. But every maintainer needs to be careful not to accidentally instantiate a transform here, and maintainers have to remember it at code review.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
discussion-neededOpen for discussionOpen for discussion