improve robustness of plot_logs function, handle single directory argument for 'logs' param #62
Conversation
- robustify logs param - checks if list, if not, deep converts to list for plotting. - expose log name as optional param (log_name = 'log.txt') to avoid internal hard-coded references - verify directories passed in are valid. Raise ValueError if not with path to broken directory shown - added parameter explanations in commments
|
Hi @lessw2020! Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
util/plot_utils.py
Outdated
| if not isinstance(logs, Iterable): | ||
| logs = [logs] |
There was a problem hiding this comment.
I think this could lead to subtle bugs.
If the user passes a single string (which is an Iterable), it will go in the next case, so converting it into a list of single-character strings.
util/plot_utils.py
Outdated
| Function to plot specific fields from training logs | ||
| :: Inputs - logs = list containing one or more directories, each w/ single log file (convert to list if needed) | ||
| - fields = which training results to plot from log file | ||
| - ewm_col = ?? |
There was a problem hiding this comment.
this is the parameter for smoothing out the plot
| ''' | ||
|
|
||
| # verify logs is list, convert if not. handle single dir, etc. *Should not have side-effects, makes deep new logs | ||
| if not isinstance(logs, list): |
There was a problem hiding this comment.
I think if we want to be more robust to the user, we might want to either enforce that it should be a Path or a List[Path], but otherwise it can become more complicated
There was a problem hiding this comment.
I agree re: becoming more complicated! (...I had started thinking about all the variety of incoming items that could be passed in, and it goes downhill fast).
Let me make a couple changes based on your feedback above, test those, and will update tonight.
Thanks much for the review and feedback!
There was a problem hiding this comment.
I've simplified it to check for either a Path, or List[Path] now per your recommendation.
First check if list, verify if Path type, then convert to list for single Path item.
Then iterate list and check each item is a)Path type and b)verify dir exists...
and then onto plotting.
I tested a bunch of combos to verify the code, and will submit tomorrow (edit - found issue with tuples...fixing).
Question - is there a specific VScode lint plugin you use for verifying/formatting to pass your CircleCI?
I was using one this weekend (flake8), but I still had to fix about 4 or 5 whitespace issues by hand, so wanted to see if there is a specific format plugin I should run?
Thanks much!
There was a problem hiding this comment.
is there a specific VScode lint plugin you use for verifying/formatting to pass your CircleCI?
I was using one this weekend (flake8), but I still had to fix about 4 or 5 whitespace issues by hand, so wanted to see if there is a specific format plugin I should run?
We just use flake8 with the some extra custom configs, so I believe you just need to point VSCode to use that and it should be enough
There was a problem hiding this comment.
Thanks @fmassa - I've updated the doc string section, and simplified the argument pre-checks to exclusively support the simpler Path and list[Path] scenarios.
I did test a number of random inputs (strings, tuples, mixes of Path and string) etc. to try and be robust on the testing as well as verify error messages are informative.
So now it supports single dir scenario, and provides err info for anything outside the bounds of supported args, as well as directory verification with info for any incorrect ones.
I also like having the log_name as an exposed, optional parameter rather than being internally hardcoded.
btw - The plots generated from this function are quite nice!
…ument for 'logs' param (facebookresearch#62) * improve robustness of plot_logs function: - robustify logs param - checks if list, if not, deep converts to list for plotting. - expose log name as optional param (log_name = 'log.txt') to avoid internal hard-coded references - verify directories passed in are valid. Raise ValueError if not with path to broken directory shown - added parameter explanations in commments * whitespace fixes for pylint * pylint flake8 update for whitespace * pylint flake8 update for whitespace, last 4 issues * code pylint missing 2 trailing wspace, fixed * blank line at end of file removed... * simplified use case to path/list[path] * 2 whitespace fixes * one last whitespace removed
Note - directory check is totally optional, comments are optional.
Core fix was really to handle single directory being passed in which was my direct use case failure and hence this PR.