Convert to reusable action #5
Conversation
For convenience, we delete `test_version`. It's easier if a reusable action's version is stored in a Git tag only, rather than in a Git tag and in the code.
We move the cli from `deciles_chart.cli` to `analysis.deciles_chart` so that all the Python code is in the same module. Consequently, this reusable action looks like a scripted action. We replace click, which isn't isn't a direct dependency of python-docker, with argparse.
We downgrade Pandas to 1.0.1, which is the version available on python-docker. This version of Pandas doesn't allow an empty DataFrame to be initialised by passing `columns` and not `data`, so we modify how empty DataFrames are initialised to fix the tests.
Require 0.0.21, which is the version available on python-docker.
This serves as an end-to-end test of the reusable action.
`ebmdatalab.charts` has a function for computing deciles. We revert this commit, if we remove that dependency. Until then, `get_deciles_table` is redundant.
On reflection, this decorator is too defensive. If `DataFrame.attrs` doesn't contain the required key, then the resulting `KeyError` should be sufficient.
Somewhat pedantic, but consistent: in a study definition the measures list contains one or more instances of the Measure class. Each instance generates a single CSV file. Each CSV file is, thus, a measure (singular) table.
This adds the tag-new-version job, which is discussed in cohort-joiner#8.
|
|
||
|
|
||
| def get_deciles_chart(measure_table): | ||
| return charts.deciles_chart(measure_table, period_column="date", column="value") |
There was a problem hiding this comment.
If we have changes to the ebmdatalab decile charts code, do you think we will push updates to the library? Or have our own code here? In relation to opensafely-core/cohort-extractor#759, we might want to be able to output the intermediate deciles tables for output checking.
There was a problem hiding this comment.
I think that we'll probably end up replacing that implementation with a new implementation, in this module. Writing out intermediate deciles tables would be one reason for replacing implementations (in this case, we could revert 31832fa). I decided to fall back to charts.deciles_chart because it's the canonical implementation.
codelists/codelists.txt
Outdated
| @@ -0,0 +1,2 @@ | |||
| opensafely/ethnicity/2020-04-27 | |||
There was a problem hiding this comment.
Currently we are loading the ethnicity codelist but not using it. For example studies we probably want to avoid circular dependencies of actions using other actions? So might we want to avoid using the cohort joiner in the decile chart action example study?
Since it is a small example study, it probably would not be too expensive to extract ethnicity for each month.
Or we could assume a reusable action hierarchy in which decile charts could use the cohort joiner, but the cohort joiner would not use decile charts.
There was a problem hiding this comment.
We should certainly remove that codelist. I don't think the example study contains a dependency on cohort-joiner, though. The path is generate_cohort > generate_measures > generate_deciles_charts.
The new ID better reflects the nature of the measure.
|
I've removed the ethnicity codelist in 98643d3 and, because I noticed it, updated the measure ID in cbd9310. Would you be happy to ✅ the PR, @ccunningham101? |
I've changed multiple files, so it's probably best to step through each commit one-by-one. I've not grouped them especially well, because I didn't want to spend my time resolving merge conflicts 😬. Essentially, though:
Most importantly: This PR is the minimum needed to get deciles-charts working as a reusable action. I know you will have excellent suggestions for configuration (e.g. #4). I know you will find the deciles charts suboptimal; the
xaxis tick labels and the legend are truncated, for example. We can address these, and more, in future PRs.Closes #1