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

Speed up of plot_discrete_cdfs by 2 orders of magnitude #306

Merged
merged 3 commits into from
Nov 3, 2023
Merged

Speed up of plot_discrete_cdfs by 2 orders of magnitude #306

merged 3 commits into from
Nov 3, 2023

Conversation

quaquel
Copy link
Owner

@quaquel quaquel commented Nov 1, 2023 •
edited
Loading

This pull request does 3 things

  1. adds a regional sensitivity analysis example
  2. speed up of plot_discrete_cdfs by 2 orders of magnitude
  3. bugfix in plot_cdfs for handling scenario_id, policy, and model

Closes #298

@quaquel
add a regional sa example
b236800
@quaquel
bug fix in plot_cdfs and speed up in plot_discrete_cdf
51ac43d 
handling of default columns in experiments dataframe changed to properly remove scenario_id as well as model and/or policy if there is only one value for these.

removal of scatter plot in plot_discrete_cdf for 2 orders of magnitude speedup
@quaquel quaquel requested a review from EwoutH November 1, 2023 08:10
@coveralls
Copy link

coveralls commented Nov 1, 2023 •
edited
Loading

Coverage Status

coverage: 80.798% (-0.1%) from 80.893%
when pulling 8060660 on regional_sa
into 81a1f49 on master.

@quaquel quaquel added this to the 2.5.0 milestone Nov 1, 2023
@EwoutH
Copy link
Collaborator

EwoutH commented Nov 1, 2023

Thanks, will review later this week.

Is this in any way breaking for current users / workflows?

@quaquel
Copy link
Owner Author

quaquel commented Nov 2, 2023

Thanks, will review later this week.

Is this in any way breaking for current users / workflows?

this change won't break anything major because it is just a visual change.

EwoutH
EwoutH reviewed Nov 2, 2023
View reviewed changes
ema_workbench/analysis/regional_sa.py Outdated Show resolved Hide resolved
ema_workbench/analysis/regional_sa.py Outdated Show resolved Hide resolved
ema_workbench/analysis/regional_sa.py Show resolved Hide resolved
ema_workbench/analysis/regional_sa.py Show resolved Hide resolved
ema_workbench/examples/regional_sa_flu.py Outdated Show resolved Hide resolved
@EwoutH
Copy link
Collaborator

EwoutH commented Nov 3, 2023

Before this PR:
278872579-4a3ac6d3-b99c-4f42-a1f7-e3b4fbc6e3e9
After this PR:
Screenshot_209

Goes from 345 seconds to 2.

And looking at the CI test times, it goes from 8 to 10 minutes to around 3 minutes.

Great work!

@EwoutH
Copy link
Collaborator

EwoutH commented Nov 3, 2023

Could you make the PR title more descriptive (for the changelog) and then feel free to (squash and) merge.

@quaquel quaquel changed the title fix for #298 Speed up of plot_discrete_cdfs by 2 orders of magnitude Nov 3, 2023
@quaquel quaquel merged commit 9090851 into master Nov 3, 2023
18 of 20 checks passed
@quaquel quaquel deleted the regional_sa branch November 3, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EwoutH EwoutH EwoutH approved these changes

Assignees
No one assigned
Labels
bug enhancement performance
Projects
None yet
Milestone
2.5.0
Development

Successfully merging this pull request may close these issues.

test_plot_cdfs test is extremely slow
3 participants
@quaquel @coveralls @EwoutH