HParams: Only filter scalar card runs by hparams when enableHparamsInTimeSeries is true
#6399
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bmd3k
approved these changes
May 22, 2023
| runSelectionMap && | ||
| runSelectionMap.get(runId) && | ||
| renderableRuns.has(runId) | ||
| (renderableRuns.has(runId) || !hparamsInTimeSeriesEnabled) |
Contributor
There was a problem hiding this comment.
Perhaps put the flag first?
(!hparamsInTimeSeriesEnabled || renderableRuns.has(runId))
Personally, I think this would be the most readable, but some might object to it as being unnecessarily verbose:
(hparamsInTimeSeriesEnabled ? renderableRuns.has(runId) : true)
Contributor
Author
There was a problem hiding this comment.
I had considered the second option but as you suggested I found it unnecessarily verbose. If you're okay with the yoda clause the 1st option you suggest does seem good to me.
| expect(data[6].run).toEqual('1 a/run7'); | ||
| })); | ||
|
|
||
| it('filters runs by hparam when enableHparamsInTimeSeries is true', fakeAsync(() => { |
Contributor
There was a problem hiding this comment.
It would be nice to refactor to share most of the setup code (perhaps in a beforeEach()) and make each test small and focused.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for features / changes
We have been discovering issues with the way the hparam domains are defined. This has lead to issues due to #6377 causing scalar cards to appear empty when one of these issues occurs. I attempted to fix this in #6393 but missed the situation where a string type hparam with more than 10 values returns an empty domain (Googlers see tb/3183400180427144287 for an example).
I am moving the new filtering logic added in by #6377 behind a flag while we fix this issue on the backend.
Screenshots of UI changes (or N/A)
Screenshots for Googlers only
Before: https://screenshot.googleplex.com/AELb2oHqhxa8UNF
After:
Feature Flag Enabled: https://screenshot.googleplex.com/AcPxsbNNXSSFGx9
Feature Flag Disabled: https://screenshot.googleplex.com/3aEXpfHrGZdzYgZ