HParams: Add new hparams selector factories #6323
Merged
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
rileyajones
force-pushed
the
hparams-filter
branch
from
85b796b to
56d6e75
Compare
Contributor
JamesHollyer
left a comment
JamesHollyer
left a comment
There was a problem hiding this comment.
Most just comments for my own understanding.
| return combineDefaultHparamFilters(defaultFilterMaps); | ||
| } | ||
|
|
||
| const getHparamsDefaultFiltersForExperiments = createSelector( |
Contributor
There was a problem hiding this comment.
So this is basically a selector with a prop right? All current calls use this?
Contributor
Author
There was a problem hiding this comment.
Yeah, exactly. It is only used a dependency of other selectors though
| ); | ||
|
|
||
| defaultFilterMaps.push(state.specs[experimentId].metric.defaultFilters); | ||
| export function getHparamFilterMapFromExperimentIds(experimentIds: string[]) { |
Contributor
There was a problem hiding this comment.
This is the one we want to use in the future?
| getMetricsDefaultFiltersForExperimentsFromExperimentIds(experimentIds), | ||
| (state, defaultFilterMap) => | ||
| getMetricFilterMapResultFunction(state, defaultFilterMap, experimentIds) | ||
| ) as MemoizedSelector<State, Map<string, IntervalFilter>>; |
Contributor
There was a problem hiding this comment.
What is the purpose of this typecast?
Contributor
Author
There was a problem hiding this comment.
I was playing with the typing there for a while, I think this can actually be removed now
JamesHollyer
approved these changes
Apr 18, 2023
dna2github
pushed a commit
to dna2fork/tensorboard
that referenced
this pull request
May 1, 2023
## Motivation for features / changes As part of the effort to bring hparams to the time series dashboard a handful of new selectors need to be created. Rather than further the practice of creating selectors with props ([which is deprecated](https://ngrx.io/guide/migration/v12)) I am choosing to create them using the recommended solution of a selector factory. Unfortunately selector factories don't play nicely with selectors dependent selectors WITH props. There are a handful of hacky solutions to this problem but I am opting to create new dependent selectors using the factory pattern. ## Technical description of changes I extracted the result functions from the existing selectors, then created new factories which call them with an additional closured parameter. ## Screenshots of UI changes None ## Detailed steps to verify changes work correctly (as executed by you) Tests should pass ## Alternate designs / implementations considered I could have just wrapped the existing selectors and then called them passing in the global state instead. I chose this approach instead with the hope of removing the old selectors in a later pr.
rileyajones
added a commit
that referenced
this pull request
May 9, 2023
## Motivation for features / changes The runs table currently contains the logic to filter runs by hparams, unfortunately, this logic is not shared with the rest of the dashboard and so filtering runs this way does not actually effect what is shown on the dashboard. ## Technical description of changes I moved out a lot of utility functions from the runs table container into common selectors. I then created three selectors 1) `getRenderableRuns` A private selector to help with the readability of `getFilteredRenderableRuns` Gets all runs associated with the provided experiment ids 2) `getFilteredRenderableRuns` Gets all runs associated with the provided experiment ids while accounting for filters. 3) `getFilteredRenderableRunsFromRoute` Retrieves the experiment ids from the route then gets the filtered runs associated with them NOTE: The utility functions are encapsulated in a utils object so that they can be spied on during testing See the PRs leading up to this #6340 #6360 #6361 #6323 #6324 ## Screenshots of UI changes (or N/A) N/A
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
As part of the effort to bring hparams to the time series dashboard a handful of new selectors need to be created. Rather than further the practice of creating selectors with props (which is deprecated) I am choosing to create them using the recommended solution of a selector factory. Unfortunately selector factories don't play nicely with selectors dependent selectors WITH props. There are a handful of hacky solutions to this problem but I am opting to create new dependent selectors using the factory pattern.
Technical description of changes
I extracted the result functions from the existing selectors, then created new factories which call them with an additional closured parameter.
Screenshots of UI changes
None
Detailed steps to verify changes work correctly (as executed by you)
Tests should pass
Alternate designs / implementations considered
I could have just wrapped the existing selectors and then called them passing in the global state instead. I chose this approach instead with the hope of removing the old selectors in a later pr.