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

Render flexible confusion matrices as expected #2523

Merged
merged 11 commits into from
Oct 5, 2022
Merged

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Oct 4, 2022

1/2 main <- this <- #2531

Closes #2516

The problem was that the revision field was not being updated by us before trying to render the plot as expected. In order to do that we have to combine the rev field with the filename in the underlying data (no change to the template for these ones).

Successfully rendering the plot then causes a separate issue which is addressed in #2531.

Screenshot:

image

Test project: https://github.com/dberenbaum/example-get-started/tree/flexible_plots (output copied as a new test fixture).

Note: +337,296 of the diff comes from extension/src/test/fixtures/plotsDiff/multiSource.ts

@mattseddon mattseddon added the bug Something isn't working label Oct 4, 2022
@mattseddon mattseddon self-assigned this Oct 4, 2022

if (isMultiView) {
fields.unshift('rev')
return JSON.parse(
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.


const data = getMultiSourceOutput()

export default data
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 uses the regular pattern if we want to include the multi-source plots in a new story then I should be able to wire them up pretty quickly.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 337569 lines exceeds the maximum allowed for the inline comments feature.

@mattseddon mattseddon marked this pull request as ready for review October 5, 2022 03:27
@mattseddon mattseddon marked this pull request as draft October 5, 2022 03:32
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 337565 lines exceeds the maximum allowed for the inline comments feature.

@mattseddon mattseddon marked this pull request as ready for review October 5, 2022 03:36
@mattseddon mattseddon marked this pull request as draft October 5, 2022 05:30
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 337565 lines exceeds the maximum allowed for the inline comments feature.

@mattseddon mattseddon marked this pull request as ready for review October 5, 2022 07:17
Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Great work!

@mattseddon mattseddon enabled auto-merge (squash) October 5, 2022 17:27
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 337565 lines exceeds the maximum allowed for the inline comments feature.

@codeclimate
Copy link

codeclimate bot commented Oct 5, 2022

Code Climate has analyzed commit 1e26fa1 and detected 0 issues on this pull request.

Too many changed lines in diff

View more on Code Climate.

@mattseddon mattseddon merged commit 6c5e2aa into main Oct 5, 2022
@mattseddon mattseddon deleted the fix-flexible-confusion branch October 5, 2022 17:42
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.

Showing wrong info for non-linear flexible plots
3 participants