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

Feature/show all metrics and table cells #798

Merged
merged 10 commits into from
Mar 30, 2022

Conversation

tynandebold
Copy link
Member

@tynandebold tynandebold commented Mar 29, 2022

Description

Resolves #773 and resolves #780.

Development notes

First, I set showDiff to true in the experiment-wrapper.js file.

Next, the real crux of this is the fillEmptyMetrics() function in the run-dataset.js file. This is what fills in the empty data, which results in the correct empty cells in the table now being displayed.

I also added some comments to the code and expanded on some that were already there, as they were lacking some arguments.

I found some missing styles on the focus states on the buttons, too, so I've added those in.

There are some smaller changes here as well, mostly resulting from auto formatting the readme and release notes file.

Lastly, I was seeing this console warning:

Browserslist: caniuse-lite is outdated. Please run:
  npx browserslist@latest --update-db

so I ran that command, and that's why there are changes in the package.json and package-lock.json files.

QA notes

To test this, select the run with the name 2021-12-16T09.45.55.269Z. That has a metric called model_author with the value of richard feynman. That differs from any of the other runs. If you select this run along with any of the others, you should be able to see that this metric is shown and the other runs that don't have this metric have the - in the table cell(s).

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

@tynandebold tynandebold requested a review from yetudada as a code owner March 29, 2022 10:26
@tynandebold tynandebold removed the request for review from yetudada March 29, 2022 10:27
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

Really nice implementation @tynandebold ! :)

Copy link
Contributor

@studioswong studioswong left a comment

Choose a reason for hiding this comment

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

I've tested this and it works well, so approving straight away.

One suggestion - it would be nice to add a test to test the expected behaviour for pre-filling / displaying the empty value as a - - perhaps something to test the fillEmptyMetrics function under run-dataset.test.js?


* As a standalone React component (for embedding Kedro-Viz in your web application).
To install the standalone React component:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I've seen the same change and suggested in Rashida's PR - I think it's worth keeping the wording(for embedding Kedro-Viz in your web application). to give context of the usage of the kedro-viz react component

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Somehow this line was moved to the line above, so it's still in there and does hopefully give the context for the users.

The lines around this one you've commented on look like this:

- As a standalone React component (for embedding Kedro-Viz in your web application).

  To install the standalone React component:

  npm install @quantumblack/kedro-viz

@@ -33,14 +33,18 @@ const resolveRunDataWithPin = (runData, pinnedRun) => {

/**
* Display the dataset of the experiment tracking run.
* @param {array} props.isSingleRun Whether or not this is a single run.
* @param {boolean} props.enableShowChanges Are changes enabled or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @param {boolean} props.enableShowChanges Are changes enabled or not.
* @param {boolean} props.enableShowChanges Indicator for enabling 'show changes' feature.

Comment on lines +68 to +70
.sort((a, b) => {
return a.localeCompare(b);
})
Copy link
Contributor

@studioswong studioswong Mar 30, 2022

Choose a reason for hiding this comment

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

Out of curiosity, I wonder if there are any specific reasons why you prefer to use localeCompare other than the normal sort method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah great question. I noticed that when I added a new metric with a capital letter (e.g. NEW_METRIC) it wasn't sorted correctly, and that's because using sort() by default, without a compare function, is case sensitive. Using localCompare fixes that :)

…ature/show-all-metrics-and-table-cells

Signed-off-by: Tynan DeBold <[email protected]>
Signed-off-by: Tynan DeBold <[email protected]>
Signed-off-by: Tynan DeBold <[email protected]>
@tynandebold
Copy link
Member Author

I've tested this and it works well, so approving straight away.

One suggestion - it would be nice to add a test to test the expected behaviour for pre-filling / displaying the empty value as a - - perhaps something to test the fillEmptyMetrics function under run-dataset.test.js?

Great idea about the test. I've added one here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants