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

Accomodate params that are lists #1818

Merged
merged 5 commits into from
Jun 2, 2022
Merged

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Jun 2, 2022

1/2 main <- this <- #1819

Closes #1814.

Comments inline.

Demo

Screen.Recording.2022-06-02.at.8.24.52.pm.mov

@@ -49,7 +49,7 @@ export type StatusesOrAlwaysChanged = StageOrFileStatuses | 'always changed'

export type StatusOutput = Record<string, StatusesOrAlwaysChanged[]>

export type Value = string | number | boolean | null
export type Value = string | number | boolean | null | number[]
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] This was the oversight, we never expected Value aka param or metric leaf values to be an array.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could it ever be a string array or even a boolean array?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, updated

@@ -27,7 +27,7 @@ const walkValueTree = (
ancestors: string[] = []
) => {
for (const [key, value] of Object.entries(tree)) {
if (value && typeof value === 'object') {
if (value && !Array.isArray(value) && typeof value === 'object') {
Copy link
Member Author

@mattseddon mattseddon Jun 2, 2022

Choose a reason for hiding this comment

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

typeof [0,1] === 'object' which meant that we then split the array into different params or metrics and got the error shown in #1814 (everywhere, table was even broken).

get(originalRow, pathArray)
pathArray => originalRow => {
const value = get(originalRow, pathArray)
if (!Array.isArray(value)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] Using this dynamic accessor means that our table display matches the CLI

image

@mattseddon mattseddon force-pushed the add-arrays-to-exp-show-output branch from f7249d7 to 61bd2e5 Compare June 2, 2022 10:18
@mattseddon mattseddon marked this pull request as ready for review June 2, 2022 10:34
@@ -49,7 +49,7 @@ export type StatusesOrAlwaysChanged = StageOrFileStatuses | 'always changed'

export type StatusOutput = Record<string, StatusesOrAlwaysChanged[]>

export type Value = string | number | boolean | null
export type Value = string | number | boolean | null | number[]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it ever be a string array or even a boolean array?

@mattseddon mattseddon force-pushed the add-arrays-to-exp-show-output branch from 43c2dd0 to 27811f4 Compare June 2, 2022 18:20
@mattseddon mattseddon enabled auto-merge (squash) June 2, 2022 18:21
@codeclimate
Copy link

codeclimate bot commented Jun 2, 2022

Code Climate has analyzed commit 27811f4 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.8% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon merged commit d245ba6 into main Jun 2, 2022
@mattseddon mattseddon deleted the add-arrays-to-exp-show-output branch June 2, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error creating an experiment when params.yaml contains a list of values
2 participants