search

HorridTom / AE-app

Analysis of publicly available NHS A&E data
Apache License 2.0
1 stars 0 forks source link

Adm vol chart feature#29 #37

Closed Yewande91 closed 1 year ago

Yewande91 commented 5 years ago

I have created a plot of the number of admission to ED (c' chart).

Changes have not been tested.

I am proposing that changes be merged to master after testing.

Resolves: #29

HorridTom commented 5 years ago

Well done @Yewande91 this looks great, the core of the issue is cracked. A few thoughts on getting it closer to integration into the app:

1) The function plot_adm_volume here is so similar to the original plot_volume function, that it seems like we should be able to handle the admissions plot using the original, rather than having a new function with lots of repeated code (DRY). This is because you've cleverly modified make_perf_series to do the hard work. In fact, running plot_volume(AE_Data, measure = "Adm_All_ED") already works in terms of the measure plotted - it is just the chart titles and labels etc. that need to be adjusted accordingly.

2) Have a look at this test, and think about how you might make a similar one for your new plot. If you want to have a go, copy and paste this test, change its name (the desc argument, see ?testthat::test_that) and modify this copied version to test your new admission volume plot. Use figures directly out of a chosen month's Excel spreadsheet (from here) as the "correct" or "expected" values for the volume(s).

3) Let's work together to integrate the chart into the app when I'm back - if you want to have a go though, look at what's going on here in the user interface, and here. This is currently a bit ugly, and I suspect we can improve it to make it easier to work with, so don't worry if it doesn't make much sense yet.

HorridTom commented 5 years ago

Best way forward will be to finish 1 and 2 above, then merge, raise a new issue for integrating this feature into the app itself, refactor the app to make plotting more tidy, then integrate.

Yewande91 commented 5 years ago

We have created a plot of the number of emergency admissions to ED (c' chart) and labelled the plot accordingly.

Changes have now been tested.

I am proposing that changes be merged to master .

Resolves: #29

HorridTom commented 5 years ago

Right this is looking really good now. Two things: a) It would be great to have a test to check that plot_volume correctly accounts for all vs type 1 only admissions. So in other words, find a month and organisation for which there are some non-type-1 admissions (as well as some type 1) and then test that the two measures are correct (they should be different in this case!). b) check what is happening with Scottish data for admissions - at the moment it looks like attendances and admissions are exactly the same. Well done, we're really close now!

HorridTom commented 5 years ago

Item (b) above is now dealt with - there is no data on admissions in the Scotland public data, so we have hidden the admissions chart if Scotland is selected as country.