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.

Sign up for GitHub

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

Jump to bottom

Issue #842 - convert plots to plotly #853

Merged
merged 7 commits into from
Apr 4, 2024
Merged

Issue #842 - convert plots to plotly #853

merged 7 commits into from
Apr 4, 2024

Conversation

calina-c
Copy link
Contributor

@calina-c calina-c commented Apr 3, 2024 •
edited
Loading

Closes #842. Closes #841.
Supersedes #850.

@calina-c calina-c changed the title Convert profit and accuracy graphs to plotly. Issue #842 - convert plots to plotly Apr 3, 2024
@calina-c calina-c mentioned this pull request Apr 3, 2024
Closed
@calina-c calina-c force-pushed the issue-842-all-plotly branch from a6559c0 to 03790e5 Compare April 3, 2024 11:22
@calina-c calina-c marked this pull request as ready for review April 3, 2024 11:36
@calina-c calina-c requested a review from idiom-bytes April 3, 2024 11:36
idiom-bytes
idiom-bytes approved these changes Apr 4, 2024
View reviewed changes
Copy link
Member

@idiom-bytes idiom-bytes left a comment

Choose a reason for hiding this comment

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

looks good, forgot both branches were in this pr
congratulations on removing more code than you added

idiom-bytes
idiom-bytes reviewed Apr 4, 2024
View reviewed changes
Copy link
Member

@idiom-bytes idiom-bytes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi calina, i tested this PR against #851, and I believe the % correct chart doesn't seem to be working.
Although in #851 it is.

I have approved the pr because it looks like it's running and not crashing... but I do not think that the chart is working and think there might be an error.

image

@idiom-bytes idiom-bytes requested review from idiom-bytes and removed request for idiom-bytes April 4, 2024 02:45
@calina-c
Copy link
Contributor Author

calina-c commented Apr 4, 2024

Hi calina, i tested this PR against #851, and I believe the % correct chart doesn't seem to be working. Although in #851 it is.

I have approved the pr because it looks like it's running and not crashing... but I do not think that the chart is working and think there might be an error.

image

I think I need to investigate more. Some states present a correct chart, others look like yours. I played around with sim a bit this morning, but I can't find a common denominator just yet. I'll open a separate issue to make sure all states present correctly.

@calina-c calina-c mentioned this pull request Apr 4, 2024
Closed
@calina-c calina-c merged commit 05387e7 into main Apr 4, 2024
5 checks passed
@calina-c calina-c deleted the issue-842-all-plotly branch April 4, 2024 08:39
@trentmc trentmc changed the title Issue #842 - convert plots to plotly Issue #842 - convert plots to plotly / streamlit Apr 30, 2024
@trentmc trentmc changed the title Issue #842 - convert plots to plotly / streamlit Issue #842 - convert plots to plotly Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idiom-bytes idiom-bytes idiom-bytes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SimEngine][Analytics][dapp] Sign off on altair vs. plotly [SimEngine] Recreate varimps graph using plotly
2 participants
@calina-c @idiom-bytes