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

Optionally output deciles tables #28

Merged
merged 2 commits into from
Apr 26, 2022

Conversation

iaindillingham
Copy link
Member

By default, we output deciles tables. Studies using deciles-charts probably won't be affected, as they probably use a glob pattern that includes ".png".

We haven't written tests for the new functions, because they are thin wrappers around ebmdatalab.charts. However, we should consider writing an integration test soon.

ad569a2 adds a terminology section. I think it's worth emphasizing that these terms are (reasonably) well-defined; as per opensafely-core/roadmap#18.

Closes #18

By default, we output deciles tables. Studies using deciles-charts
probably won't be affected, as they probably use a glob pattern that
includes ".png".

We haven't written tests for the new functions, because they are thin
wrappers around `ebmdatalab.charts`. However, we should consider writing
an integration test soon.
@ccunningham101
Copy link

Thinking about where we might go from here with redaction
Ideally we could do redaction on the deciles table, so we would check that there are >=5 people per decile rather than >=5 people per practice (if done at the measure file level)

Right now for both the table and the chart, we use the same measures table, but what do you think about either workflow

  1. We include redaction code after the invocation of get_deciles_table and feed the result into get_deciles_chart
  2. We have a separate redaction reusable action that would contain code similar to the measures framework small number suppression, but could be applied to an arbitrary table outside the measures framework. Here, we might want the option for this reusable action to take in an already redacted decile table rather than a measures file

@iaindillingham
Copy link
Member Author

I think that both workflows are worth investigating further 🙂

The first is easier to reason about than the second; we know roughly where to redact; we know roughly what to redact. However, we'd probably have to refactor the reusable action to remove the dependency on ebmdatalab.charts, because charts.deciles_chart generates both the deciles table and the deciles chart. If we didn't, then generating a deciles chart from a redacted deciles table would be a pain. (I'd like to investigate pytest-mpl, as it would be nice to be able to assert that the refactoring was correct.)

The second is harder to reason about. As you say, it would have to apply to an arbitrary table. But semantic information is central to redaction, so we would have to pass it, alongside the arbitrary table, which would make it harder to maintain the low/no config of reusable actions.

The good news is that the first leads to the second. Could you create a new issue that describes the types of redaction that would be useful?

@iaindillingham iaindillingham merged commit ec1a69c into main Apr 26, 2022
@iaindillingham iaindillingham deleted the iaindillingham/optionally-output-deciles-tables branch April 26, 2022 08:09
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.

Write out the data for a deciles chart
2 participants