Skip to content

Make the clusterPlots notebook consistent with telemetry schema - #68

Open
Pooja Nilangekar (poojanilangekar) wants to merge 3 commits into
microsoft:mainfrom
poojanilangekar:cluster-plots
Open

Make the clusterPlots notebook consistent with telemetry schema#68
Pooja Nilangekar (poojanilangekar) wants to merge 3 commits into
microsoft:mainfrom
poojanilangekar:cluster-plots

Conversation

@poojanilangekar

Copy link
Copy Markdown
Contributor

Making the clusterPlots notebook consistent with execTimePlots and storagePlots notebooks.

Comment thread metrics/notebooks/clusterPlots.ipynb Outdated
"\n",
"# Retrieve relevant data from duckdb.\n",
"EXP_DATA = retrieve_experiment_df(con, EXPERIMENT_ID, EXPERIMENT_START_TIME)\n",
"EXP_DATA = pd.DataFrame()\n",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For cluster plots, we only plot one experiment at a time, thus we only have one experiment id (EXPERIMENT_ID = "" instead of EXPERIMENT_ID = [""] like in the other notebooks), so you wouldn't actually need the for-loop here. The reason is that you're plotting different phases as different lines in the plot (vs different experiments as different lines in the plot which is what we do in the latency plots), thus you can actually only plot one experiment at a time. If you have any suggestions on how to visualize multiple experiments in the same plot, let me know! :)

@ashvina

Copy link
Copy Markdown
Contributor

Hi Pooja Nilangekar (@poojanilangekar), thanks for the PR and addressing my review comments.
For context, could you please add a short description of the actual issue you are observing, for e.g. in running the notebook or in extending the notebook for new functionality. It would help in ensuring it is not repeated.
Otherwise LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants