-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix calibration scripts for Pandas v2 #1159
Conversation
src/scripts/calibration_analyses/analysis_scripts/analysis_demography_calibrations.py
Outdated
Show resolved
Hide resolved
src/scripts/calibration_analyses/analysis_scripts/analysis_demography_calibrations.py
Outdated
Show resolved
Hide resolved
src/scripts/calibration_analyses/analysis_scripts/analysis_demography_calibrations.py
Outdated
Show resolved
Hide resolved
Thanks @giordano These scripts are used really heavily in analyses and the dev server. UPDATE: Worked out that I was looking at a different script. You're quite right about the errors on this one and the dev server seems that it hasn't caught up with the changes in the pinned pandas version (so no errors yet, but they will come but it gets to it!) |
What version of Pandas are you using? They work for me with Pandas v1.2.2, but not v2.0.3, which is now in Line 136 in 72aaf87
|
hang-on. Update coming...... I've found my error |
I;m on that version, and do indeed reproduce the error (and the dev server will too when I gets to the change in version, I presume) |
@giordano and @matt-graham -- is this script running well on your end now? Am I safe to merge it in and use it on my branches? |
c90e982
to
9ef8507
Compare
No, with this PR I only addressed the first few errors I was facing, but then I got many more at which point I gave up. I updated this PR to replace the |
Ok, thanks. I'll get it working. |
… on index within a seperate function
…avoid errors and warnings
…hort treatment_id and the default colour scheme
Hi @giordano and @matt-graham |
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.
@tbhallett - thanks this looks good. I've verified that with your changes the src/scripts/calibration_analyses/analysis_scripts/analysis_all_calibration.py
script runs to completion using the last full set of long_run_all_disease
scenario results from the dev server (2023-09-20_090745_b9e157f95_021
) in an environment with pandas v2.
I've made some suggested minor changes around removing plt.show()
calls which I think should be unnecessary if plot is being saved and closed immediately, and some other small changes. I think there are also some other plt.show
instances in the scripts which are touched by changes here so I can't directly make suggestions on. I would say if we're assuming this script is for producing save figure files rather than interactively displaying, it would be best to remove all plt.show
instances to simplify running on machines without an X server for displaying plots.
...calibration_analyses/analysis_scripts/analysis_cause_of_death_and_disability_calibrations.py
Outdated
Show resolved
Hide resolved
...calibration_analyses/analysis_scripts/analysis_cause_of_death_and_disability_calibrations.py
Outdated
Show resolved
Hide resolved
...calibration_analyses/analysis_scripts/analysis_cause_of_death_and_disability_calibrations.py
Show resolved
Hide resolved
...calibration_analyses/analysis_scripts/analysis_cause_of_death_and_disability_calibrations.py
Outdated
Show resolved
Hide resolved
...pts/calibration_analyses/analysis_scripts/analysis_compare_appt_usage_real_and_simulation.py
Outdated
Show resolved
Hide resolved
src/scripts/calibration_analyses/analysis_scripts/analysis_demography_calibrations.py
Outdated
Show resolved
Hide resolved
src/scripts/calibration_analyses/analysis_scripts/analysis_demography_calibrations.py
Outdated
Show resolved
Hide resolved
src/scripts/calibration_analyses/analysis_scripts/analysis_demography_calibrations.py
Outdated
Show resolved
Hide resolved
src/scripts/calibration_analyses/analysis_scripts/plot_appt_use_by_hsi.py
Outdated
Show resolved
Hide resolved
…se_of_death_and_disability_calibrations.py Co-authored-by: Matt Graham <[email protected]>
…se_of_death_and_disability_calibrations.py Co-authored-by: Matt Graham <[email protected]>
…se_of_death_and_disability_calibrations.py Co-authored-by: Matt Graham <[email protected]>
…ography_calibrations.py Co-authored-by: Matt Graham <[email protected]>
Thanks so much for the close review and the good tips, @matt-graham I see what you mean about the use of |
@matt-graham - when you run this (
|
Yeah I also get those warnings which are I think from the lines TLOmodel/src/tlo/population.py Lines 64 to 67 in dcdf014
This started appearing on the shift to pandas v2. I'm not sure, but I think the performance issue mentioned is with respect to the cost of creating a dataframe with lots of columns by inserting one by one rather than adding altogether, rather than having an implication for the subsequent cost of accessing data in that dataframe (which would be much more of an issue in our case), but it would still be worth fixing this to avoid the warnings as this should be quick to do. I'll raise this as a separate issue
Thanks for making the edits. With the latest changes script still working locally for me on downloaded scenario outputs, and now no figure windows are appearing 🎉 |
I'm trying to run the calibration with pandas v2, but I'm facing a string of errors, trying to fix them one by one. Opening as a draft for the time being, as I'm not done yet with all the fixes (I'm also not entirely sure they're all correct).
Do I take correctly these scripts aren't currently tested?