-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Group colours by regex of the experiment name. #6846
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR does not follow our review best practices. Can you please first 1) remove (or split out) the unnecessary changes like BUILD file reordering and 2) at least split the made code into two separate PRs (e.g. redux code first, then the rest) (go/small-cls)
@hoonji I will certainly try then remove the formatting of the BUILD files in this PR. Regarding the change you requested.
Does it just mean separate the formatting of the files and added feature code? |
@AnuarTB I mean separating out the actions, effects, reducers and utils into a separate PR, and putting the component/container logic in a followup PR. I think it's silly to spend too much effort just splitting code, but we should prefer it in cases like this where a split is easy and natural. |
+1 This PR is quite large. It would be very helpful to review if you separated this PR into two parts. |
Ok so If I understood correctly, I should separate store, actions and reducers into first PR and then the views in the second PR? If that's so I will separate now, I was just confused on how should i separate this CL, as it is one big coherent block of code. |
Yeah Exactly
How about putting all the other code in one PR, and the code under |
Have separated Model and View parts to separate PRs. When this PR is submitted, will submit: #6847 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congratulations on adding a new feature to TB!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid to me. Thanks for applying this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
**NOTE**: This PR is a continuation of #6846. ## Motivation for features / changes Users want to color runs by the regex string of the corresponding to runs experiment names. ## Technical description of changes - Added new GroupBy type, REGEX_BY_EXP. - Added a dropdown in dialog window, so users could select between regex for run name or experiment name. ## Screenshots of UI changes (or N/A) N/A since internal change. ## Detailed steps to verify changes work correctly (as executed by you) - Run tensorboard.corp server. - Click on color grouping icon and select `Regex`. - Select `Experiment Name` in dropdown. ## Alternate designs / implementations considered (or N/A) N/A
Motivation for features / changes
Users want to color runs by the regex string of the corresponding to runs experiment names.
Technical description of changes
Screenshots of UI changes (or N/A)
N/A since internal change.
Detailed steps to verify changes work correctly (as executed by you)
Regex
.Experiment Name
in dropdown.Alternate designs / implementations considered (or N/A)
N/A