Skip to content
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

Recreate the Experiments Parallel Coordinates Graph #1974

Merged

Conversation

elenzio9
Copy link
Contributor

@elenzio9 elenzio9 commented Oct 19, 2022

In this PR:

  • Remove the existing Parallel Graph
  • Introduce the new Parallel Graph
  • Create unit test for the Parallel Graph

parallel-graph

Refs #1879

Signed-off-by: Elena Zioga [email protected]

* Import echarts module and ngx-echarts directive for Echarts.

Signed-off-by: Elena Zioga <[email protected]>
* Remove trials graph component.

Signed-off-by: Elena Zioga <[email protected]>
* Create a new component that uses Echarts Parallel Graph.

Signed-off-by: Elena Zioga <[email protected]>
@johnugeorge
Copy link
Member

Curious why CI tests haven't run

@johnugeorge
Copy link
Member

/ok-to-test

@tenzen-y
Copy link
Member

Curious why CI tests haven't run

GitHub seems to have incidents with API requests.

https://www.githubstatus.com/incidents/4vr2qz8lgdmq

@tenzen-y
Copy link
Member

@kimwnasptd Could you help us review this PR?

/assign @kimwnasptd

Copy link
Member

@kimwnasptd kimwnasptd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tenzen-y @johnugeorge aside form a small nit the changes look good to me.

We've worked together with @elenzio9 for this one and I have already done an internal pass at this.

Next steps will be to fix all the unit tests and have a basic GH Action that will be running these tests with each PR. We've already done some ground work for the common code kubeflow/kubeflow#6676 and will continue with a subsequent PR

/lgtm
/approve

@@ -253,4 +253,10 @@ export class ExperimentDetailsComponent implements OnInit, OnDestroy {
return StatusEnum.CREATED;
}
}

private showGraphF(response: string): boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, let's use a slightly more descriptive name here. Should this be shoGraphFn instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure! I fixed it! Let me know if it works for you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@google-oss-prow google-oss-prow bot added the lgtm label Oct 21, 2022
@kimwnasptd
Copy link
Member

kimwnasptd commented Oct 21, 2022

/hold

EDIT: Added the LGTM and APPROVE labels by mistake

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elenzio9, kimwnasptd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Make the wrapper component use the new graph.
* Show the graph when at least one trial is completed.

Signed-off-by: Elena Zioga <[email protected]>
* Create unit test for Parallel Graph.

Signed-off-by: Elena Zioga <[email protected]>
@elenzio9 elenzio9 force-pushed the katib-recreate-experiments-graph branch from 6f541aa to c305464 Compare October 21, 2022 08:54
@google-oss-prow google-oss-prow bot removed the lgtm label Oct 21, 2022
@kimwnasptd
Copy link
Member

/lgtm
/hold cancel

@google-oss-prow google-oss-prow bot merged commit 38e2017 into kubeflow:master Oct 21, 2022
@kimwnasptd kimwnasptd deleted the katib-recreate-experiments-graph branch October 21, 2022 09:55
@elenzio9 elenzio9 mentioned this pull request Oct 21, 2022
@orfeas-k orfeas-k mentioned this pull request Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants