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

Renal reg #781

Merged
merged 14 commits into from
Apr 20, 2022
Merged

Renal reg #781

merged 14 commits into from
Apr 20, 2022

Conversation

CarolineMorton
Copy link
Contributor

Start of adding the renal registry to the backend.

This adds with_record_in_UKRR() to patients.py for the
purpose of passing through arguments.
Copy link
Contributor

@rebkwok rebkwok left a comment

Choose a reason for hiding this comment

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

Looks good so far - just a few minor comments

cohortextractor/patients.py Outdated Show resolved Hide resolved
cohortextractor/patients.py Outdated Show resolved Hide resolved
cohortextractor/patients.py Outdated Show resolved Hide resolved
cohortextractor/patients.py Outdated Show resolved Hide resolved
def with_record_in_UKRR(
# picks dataset held by UKRR
from_dataset,
returning,
Copy link
Contributor

Choose a reason for hiding this comment

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

pretty much everywhere else in patients.py we use keyword args only - I'm assuming that's to prevent users putting positional args in the wrong order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I completely understand this comment. Do you mean change the order of them or remove? We need to be able to identify the dataset in the UKRR somehow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just meant to make them keywords instead of positional args. It means users can't mix up the order and specify returning where we think we're getting from_dataset.

Like:

def with_record_in_UKRR(
    # picks dataset held by UKRR
    from_dataset=None,
    returning="binary_flag",
    ...

I think elsewhere we generally default to binary_flag for returning. from_dataset could have a default if it makes sense to default to one of them, or None and we raise an exception if they don't provide it


assert_results(
study.to_dicts(convert_to_strings=False),
renal_registry_2019_prev=[1, 0]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think binary_flag returns strings (but the test output will tell us that, I guess :) )

tests/tpp_backend_setup.py Outdated Show resolved Hide resolved
ICHD, HHD, HD, PD, Tx
* "treatment_modality_prevalence" - the treatment modality from the prevalence data
* "latest_creatinine" - most recent creatinine held by UKRR
* "latest_egfr" - most recent eGFR held by UKRR
Copy link
Contributor

Choose a reason for hiding this comment

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

If it makes more sense, we can map these things to more user-friendly labels (same for the dataset names). I'm not sure if it's helpful here, but some of the other methods do (e.g. patients_admitted_to_hospital).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's talk about this

* "latest_creatinine" - most recent creatinine held by UKRR
* "latest_egfr" - most recent eGFR held by UKRR
on_or_before: date of interest as a string with the format `YYYY-MM-DD`. Filters results to measurements
on or before the given date (as defined by `date_filter_column`).
Copy link
Contributor

Choose a reason for hiding this comment

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

copy/paste error (and in the next 2 args)? There's no date_filter_column

on_or_before=None,
on_or_after=None,
between=None,
include_date_of_match=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this arg?

# Date filtering: date limits
on_or_before=None,
on_or_after=None,
between=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

We only need between here - process_covariate_definitions.handle_time_period_options converts any on_or_before or on_or_after args into a between one before it gets to the backend.

elif returning == "latest_egfr":
column_definition = "eGFR_ckdepi"
else:
column_definition = returning
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be nicer as another mapping:

column_mapping = {
    "binary_flag": 1,
    "treatment_modality_start": "mod_start",
    ...
}
column_definition = column_mapping.get(returning)

And then we can also check and raise an exception if the returning value isn't one we expected

renal_centre="AD100",
rrt_start="2019-01-01",
mod_start="RT",
mod_prev="NULL",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely confident about this, but I think you want None here, and sqlalchemy will make it a null in the db. (Unless nulls in the real data are represented by the string "NULL".)

@CarolineMorton CarolineMorton marked this pull request as ready for review April 13, 2022 20:29
Copy link
Contributor

@rebkwok rebkwok left a comment

Choose a reason for hiding this comment

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

I left a few minor suggestions/questions, but this looks good 👍

THere's something formatting-related that black is complaining about atm

cohortextractor/patients.py Outdated Show resolved Hide resolved
cohortextractor/tpp_backend.py Show resolved Hide resolved
tests/test_tpp_backend.py Outdated Show resolved Hide resolved
tests/test_tpp_backend.py Show resolved Hide resolved
tests/test_tpp_backend.py Outdated Show resolved Hide resolved
@CarolineMorton CarolineMorton merged commit d25a266 into main Apr 20, 2022
@CarolineMorton CarolineMorton deleted the renal-reg branch April 20, 2022 15:08
@CarolineMorton CarolineMorton mentioned this pull request Apr 21, 2022
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.

None yet

2 participants