Consolidate collectCustomPlots#3466
Conversation
* have the code just have separate funcs for values creation
| return iteration | ||
| } | ||
| const group = exp.name || exp.label | ||
| const maxEpoch = exp.checkpoints.length + 1 |
There was a problem hiding this comment.
I just realized that using this way to get iterations breaks modified experiments. I need to understand a bit more on how iterations are originally gathered and see if it's even a good idea to get iterations through experiments...
If not, I either need to see if we can gather both plots through output data or rethink how to consolidate when plot plots require two different kinds of data.
There was a problem hiding this comment.
After a discussion with Matt, we decided to keep the current implementation for now, since with a future change in the exp show output, it's likely that the current way of collecting iterations will no longer work anyway.
|
|
||
| public transformAndSetExperiments(data: ExperimentsOutput) { | ||
| this.recreateCustomPlots(data) | ||
| public transformAndSetExperiments() { |
There was a problem hiding this comment.
[I] As discussed on the call this morning.
I think this can now be dropped and recreateCustomPlots can be renamed/called instead of getCustomPlots. It will get called from sendWebviewMessage and it can be passed the selected revisions so that we only need to process the required experiments. Instead of processing them all and then filtering them afterwards.
There was a problem hiding this comment.
Since this pr already has approval and all other comments have been addressed, I completed this change in a separate pr, #3491
extension/src/plots/model/index.ts
Outdated
| ? this.experiments | ||
| .getExperimentsWithCheckpoints() | ||
| .filter(({ checkpoints }) => !!checkpoints) | ||
| : this.experiments.getExperiments() |
There was a problem hiding this comment.
[B] collectCustomPlots should take all experiments regardless of whether or not they have checkpoints. I think the best thing to do here would be call this.experiments.getExperimentsWithCheckpoints() all the time as the checkpoints part of collectCustomPlots will have a type guard in it anyway?
There was a problem hiding this comment.
I think the best thing to do here would be call this.experiments.getExperimentsWithCheckpoints() all the time as the checkpoints part of collectCustomPlots will have a type guard in it anyway?
This ternary expression is only there to filter out commit data. Technically, we are planning to add commit data to the metric vs param plots (see #3373) but I chose to keep things filtered since this is a housekeeping pr. I don't mind just going ahead and adding the commit data in this pr though!
There was a problem hiding this comment.
Now that I think about it, there is definitely an easier way to filter out experiment commits. Will update how we filter for now!
There was a problem hiding this comment.
Now that I think about it, there is definitely an easier way to filter out experiment commits. Will update how we filter for now!
Originally was thinking I could filter by type or commit but looks like either of those could be unavailable. Ended up just renaming the variable to make things more clear.
| return iteration | ||
| } | ||
| const group = exp.name || exp.label | ||
| const maxEpoch = exp.checkpoints.length + 1 |
| expect(data).toStrictEqual(checkpointPlotsFixture) | ||
| }) | ||
|
|
||
| it('should provide a continuous series for a modified experiment', () => { |
There was a problem hiding this comment.
[F] This logic has been removed on purpose. Change is being discussed in [WIP] exp: refactor show behavior PR. Link is further down.
* fullValues to values * use typeguard on ExperimentsWithDefinedCheckpoints
3 participants
2/3
main<= #3404 <= this <= #3491Resolves #3404 (comment)