Skip to content

Customdummy - #34

Open
wjchulme wants to merge 9 commits into
mainfrom
customdummy
Open

Customdummy#34
wjchulme wants to merge 9 commits into
mainfrom
customdummy

Conversation

@wjchulme

Copy link
Copy Markdown
Contributor

No description provided.

@sebbacon sebbacon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks for doing it - really useful to have a worked example.

Couple of typo-type things, and a suggestion for improving clarity.

Comment thread rmarkdown/Rdemo.Rmd Outdated
You can also provide your own dummy data. This can be created however you like, as long as it contains the same variables that would be present based on the specification in the study definition. To use a custom dummy dataset, you can [designate the file as the dummy data in the cohort extractor action](https://docs.opensafely.org/study-def-expectations/#providing-your-own-dummy-data). This allows you to bypass the expectations framework provided by OpenSafely, which can sometimes be quite restrictive. You can make the variables, and the relationships between them, as realistic as necessary to test arbitrarily complex analyses.

[![](../released-ouput/plots/plot_stppop_map.png "registration count by STP"){width="50%"}](https://github.com/opensafely/os-demo-research/blob/master/released-ouput/plots/plot_stppop_map.png)
See the (https://github.com/opensafely/os-demo-research/blob/master/analysis/custom_dummy_data_stp.R) and the [custom dummy data section in the `project.yaml`](https://github.com/opensafely/os-demo-research/blob/master/project.yaml#85) to see custom dummy data in action.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First link is missing link text.

You should make the second link point to a line is a specific commit (e.g. use "copy permalink" from the line-selection kebab menu) rather than a branch, so that it remains correct over time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make the second link point to a line is a specific commit

I was waiting for the commit to be on master - though I guess it doesn't matter

Comment thread rmarkdown/Rdemo.Rmd Outdated
Comment thread rmarkdown/Rdemo.Rmd Outdated
As the script used to create the map uses a [shape file](https://github.com/opensafely/os-demo-research/blob/master/lib/STPshapefile.json), the dataset needs to contain the real STP codes so that the shape file matches the data. Running the script on dummy data where the STP codes are "STP1" "STP2" and "STP3", as above, wouldn't work. So the dummy data needs to usee real STP codes.

The outputs look like this:
One way to do this is outlined in an alternative study definition, [`study_definition_1_stppop_v2.py`](https://github.com/opensafely/os-demo-research/blob/master/analysis/study_definition_1_stppop_v2.py), where the _expectations_ for the `stp` variable is based on real STP codes imported from a CSV file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can make the fact there are alternatives clearer with a bit of renaming?

The v1 and v2 and v3 stuff is a little confusing; partly because you're having to do some somersaults to work around having alternative execution paths with identical outputs.

Might it be easier to explain by having two different project.yamls that are almost exactly the same?

Or a commented-out expectations-only version that (in the comments) can say "uncomment this and comment out that in order to try the expectations-only path"?

I'd do a better job of checking this myself if I had my normal computer for checkouts etc, so I'm being a bit vaguer than I'd like.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is difficult. It might even be better to have a completely new example to showcase custom dummy data, so that there's no v2 and v3 nonsense.

Initially _v2 was _map. We were keen to include a map in the demo because it's simple and striking. But it requires real STP codes which is just a little bit too complicated to digest on a first pass.

It it possible to have two project.yamls in the same repo? how does the CLI / job server choose which to use?

Maybe just commenting in/out the dummy_data_file line the the extractor action is good enough.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It it possible to have two project.yamls in the same repo? how does the CLI / job server choose which to use?
Right now, you can't direct the framework to use a different one. You'd just have to say in the docs (README & Rmd commentary & comments?) that to use the alternative one you need to rename them...

Not sure of the best answer but I think I prefer either the rename or the comment-out-in-yaml approaches to the current one...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've redone it so that there's no awkward repeating of actions or the need to comment anything out. Basically the maps example is back the way it was before (with files and scripts using the _maps suffix) and I've created a completely new example for explaining custom dummy data

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