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

Ongoing developments #20

Merged
merged 14 commits into from
Jan 17, 2023
Merged

Ongoing developments #20

merged 14 commits into from
Jan 17, 2023

Conversation

philmain28
Copy link
Collaborator

Branch contains changes proposed here as they are implemented:
#19

Along with enhancements/issues fixes.

@philmain28
Copy link
Collaborator Author

Added functionality to exclude patients who have a DoD elsewhere in the UKRDC for example as a patient record created by TRACING. I have also included a switch to easily turn this on or off.

@jtc42
Copy link
Contributor

jtc42 commented Dec 7, 2022

Added functionality to exclude patients who have a DoD elsewhere in the UKRDC for example as a patient record created by TRACING. I have also included a switch to easily turn this on or off.

I'm assuming there's a fairly substantial performance hit associated with this?

@philmain28
Copy link
Collaborator Author

philmain28 commented Dec 7, 2022

Yeah there is but the demographics stuff calculates pretty quickly anyway. The performance can be improved by uncommenting the line which restricts it to looking in tracing.

@codecov
Copy link

codecov bot commented Jan 6, 2023

Codecov Report

Base: 73.21% // Head: 69.15% // Decreases project coverage by -4.06% ⚠️

Coverage data is based on head (9088e1b) compared to base (ff77a25).
Patch coverage: 88.37% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
- Coverage   73.21%   69.15%   -4.07%     
==========================================
  Files          11       13       +2     
  Lines         336      389      +53     
==========================================
+ Hits          246      269      +23     
- Misses         90      120      +30     
Flag Coverage Δ
3.10 69.15% <88.37%> (-4.07%) ⬇️
3.11 69.15% <88.37%> (-4.07%) ⬇️
3.7 69.15% <88.37%> (-4.07%) ⬇️
3.8 69.15% <88.37%> (-4.07%) ⬇️
3.9 69.15% <88.37%> (-4.07%) ⬇️
unittests 69.15% <88.37%> (-4.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
ukrdc_stats/biomarkers.py 0.00% <ø> (ø)
ukrdc_stats/calculators/abc.py 100.00% <ø> (ø)
ukrdc_stats/descriptions.py 100.00% <ø> (ø)
ukrdc_stats/utils.py 60.00% <33.33%> (ø)
ukrdc_stats/calculators/demographics.py 88.40% <76.47%> (-4.93%) ⬇️
ukrdc_stats/calculators/dialysis.py 92.91% <96.77%> (+1.32%) ⬆️
ukrdc_stats/models/generic_2d.py 100.00% <100.00%> (ø)
ukrdc_stats/models/networks.py 0.00% <0.00%> (-100.00%) ⬇️
ukrdc_stats/__init__.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@philmain28
Copy link
Collaborator Author

This is nearly ready to merge and contains a fairly substantial set of changes (major release?). I suspect there is going to be a big performance hit associated with it.

Still left to do:

  • Profile the speed
  • Update the descriptions to accurately reflect the current status of the code

@jtc42
Copy link
Contributor

jtc42 commented Jan 6, 2023

If you could profile the performance hit before merging that'd be ideal. We can't afford a substantial hit to the dialysis stats without potentially requiring pre-computing. Which we can do, but I'd love not to have to for now until we have a better idea of how people will use this.

@philmain28
Copy link
Collaborator Author

philmain28 commented Jan 10, 2023

dialysis stats now calculates in 2-20 secs (on my desktop but it would probably be more on the app server). There also seems to be quite a lot of variation based on the postgres optimizations and how heavily the database is being used.

I will think about ways to speed up the code but any suggestions would be welcome.

@philmain28 philmain28 closed this Jan 11, 2023
@philmain28 philmain28 reopened this Jan 11, 2023
@philmain28 philmain28 requested a review from jtc42 January 11, 2023 19:06
@philmain28
Copy link
Collaborator Author

Summary of changes

dialysis calculator

  • Sankey plots removed and replaced with pie charts (or more precisely the underlying pydantic data structures have been switched out)
  • Stats are now calculated for each of the satellite units
  • Replaced some of the hard coded things with lookups

demographics

  • Included look up against staging

demo

  • Updated dialysis notebook to demo new data structure
  • Included Benchmarks in notebook and added various switches into the calculators to allow benchmarking

@philmain28 philmain28 marked this pull request as ready for review January 17, 2023 14:06
@philmain28 philmain28 merged commit e661d01 into master Jan 17, 2023
@philmain28 philmain28 deleted the development branch January 17, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants