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

FLOPs and ModuleSummary Documentation #152

Closed
BenjaminHelyer opened this issue Apr 27, 2023 · 2 comments
Closed

FLOPs and ModuleSummary Documentation #152

BenjaminHelyer opened this issue Apr 27, 2023 · 2 comments

Comments

@BenjaminHelyer
Copy link
Contributor

📚 The doc issue

I recently discovered this library and it looks very promising! As a newcomer, I have a couple of specific ideas about module documentation and hope they might be able to benefit the project in some way. Please excuse me if these suggestions are overly-specific: they just encapsulate a few small roadblocks that I encountered when I was first playing with the library.

Specifically, I am interested in the FLOPs and module evaluation advertised in the goals in Issue #82 . Here are my specific suggestions:

  1. Minor fix for the docstring of FlopTensorDispatchMode. As it currently stands, the example code won't run due to a small variable naming issue. I'm making a quick PR to this effect (hope that's okay). Since flops.py was linked in Issue Comparison to torchmetrics #82 , it was the first file I stepped into in the library, so I suspect others may have the same experience as me in the future.

  2. Update docs with recommended usage for obtaining number of FLOPs and module summary. I wasn't sure at the beginning of the preferred way to get the number of FLOPs. The first thing I tried was the example code in FlopTensorDispatchMode, but it seems like the preferred way would be to use torcheval.tools.get_summary_table on a module. In this vein, I think we should direct users away from FlopTensorDispatchMode somehow.

Thanks for considering these suggestions; I hope to continue to be engaged in this!

Suggest a potential alternative/fix

  1. PR will be made shortly after this issue is posted.

  2. My feeling is that when users search 'FLOPs' in the docs or wander into flops.py, they should be directed towards torcheval.tools.get_summary_table. Add example snippet to ModuleSummary docs, possibly in torcheval.tools.get_summary_table. Also possibly add guidance in the docstring of FlopTensorDispatchMode to direct users toward ModuleSummary.

@ananthsub
Copy link
Contributor

ananthsub commented Apr 27, 2023

Thanks for the feedback @BenjaminHelyer !

For point 2, the flops code is designed to be used independently. The module summary adds additional metadata that may not be desirable for all users. However, we can certainly point people from the FLOPs tools to the module summary if they're looking for an expanded set of information about their modules.

@BenjaminHelyer
Copy link
Contributor Author

Awesome, thanks so much for the explanation @ananthsub ! In that case, I think the documentation is good for both as it stands for now.

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 a pull request may close this issue.

2 participants