-
Notifications
You must be signed in to change notification settings - Fork 113
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
Changes from 4 commits
57e0045
49e8d69
e62db82
5886bb6
3625a8a
91272b6
617e638
4083d72
bb6d957
cebdd28
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
* @param {boolean} props.isSingleRun Whether or not this is a single run. | ||||||
tynandebold marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
* @param {string} props.pinnedRun ID of the pinned run. | ||||||
* @param {array} props.selectedRunIds Array of strings of runIds. | ||||||
* @param {array} props.trackingData The experiment tracking run data. | ||||||
*/ | ||||||
const RunDataset = ({ | ||||||
enableShowChanges, | ||||||
isSingleRun, | ||||||
trackingData = [], | ||||||
pinnedRun, | ||||||
enableShowChanges, | ||||||
selectedRunIds, | ||||||
trackingData = [], | ||||||
}) => { | ||||||
return ( | ||||||
<div | ||||||
|
@@ -61,15 +65,18 @@ const RunDataset = ({ | |||||
size="large" | ||||||
> | ||||||
{Object.keys(data) | ||||||
.sort() | ||||||
.sort((a, b) => { | ||||||
return a.localeCompare(b); | ||||||
}) | ||||||
Comment on lines
+68
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
.map((key, rowIndex) => { | ||||||
return buildDatasetDataMarkup( | ||||||
key, | ||||||
dataset.data[key], | ||||||
rowIndex, | ||||||
isSingleRun, | ||||||
pinnedRun, | ||||||
enableShowChanges | ||||||
enableShowChanges, | ||||||
selectedRunIds | ||||||
); | ||||||
})} | ||||||
</Accordion> | ||||||
|
@@ -85,17 +92,21 @@ const RunDataset = ({ | |||||
* @param {array} datasetValues A single dataset array from a run. | ||||||
* @param {number} rowIndex The array index of the dataset data. | ||||||
* @param {boolean} isSingleRun Whether or not this is a single run. | ||||||
* @param {string} pinnedRun ID of the pinned run. | ||||||
* @param {boolean} enableShowChanges Are changes enabled or not. | ||||||
* @param {array} selectedRunIds Array of strings of runIds. | ||||||
*/ | ||||||
function buildDatasetDataMarkup( | ||||||
datasetKey, | ||||||
datasetValues, | ||||||
rowIndex, | ||||||
isSingleRun, | ||||||
pinnedRun, | ||||||
enableShowChanges | ||||||
enableShowChanges, | ||||||
selectedRunIds | ||||||
) { | ||||||
// function to return new set of runData with appropriate pin from datasetValues and pinnedRun | ||||||
const runDataWithPin = resolveRunDataWithPin(datasetValues, pinnedRun); | ||||||
const updatedDatasetValues = fillEmptyMetrics(datasetValues, selectedRunIds); | ||||||
const runDataWithPin = resolveRunDataWithPin(updatedDatasetValues, pinnedRun); | ||||||
|
||||||
return ( | ||||||
<React.Fragment key={datasetKey + rowIndex}> | ||||||
|
@@ -144,4 +155,34 @@ function buildDatasetDataMarkup( | |||||
); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Fill in missing run metrics if they don't match the number of runIds. | ||||||
* @param {array} datasetValues Array of objects for a metric, e.g. r2_score. | ||||||
* @param {array} selectedRunIds Array of strings of runIds. | ||||||
* @returns Array of objects, the length of which matches the length | ||||||
* of the selectedRunIds. | ||||||
*/ | ||||||
function fillEmptyMetrics(datasetValues, selectedRunIds) { | ||||||
if (datasetValues.length === selectedRunIds.length) { | ||||||
return datasetValues; | ||||||
} | ||||||
|
||||||
const metrics = []; | ||||||
|
||||||
selectedRunIds.forEach((id) => { | ||||||
const foundIdIndex = datasetValues.findIndex((item) => { | ||||||
return item.runId === id; | ||||||
}); | ||||||
|
||||||
// We didn't find a metric with this runId, so add a placeholder. | ||||||
if (foundIdIndex === -1) { | ||||||
metrics.push({ runId: id, value: null }); | ||||||
} else { | ||||||
metrics.push(datasetValues[foundIdIndex]); | ||||||
} | ||||||
}); | ||||||
|
||||||
return metrics; | ||||||
} | ||||||
|
||||||
export default RunDataset; |
There was a problem hiding this comment.
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 componentThere was a problem hiding this comment.
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: