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

i28 generalise upper and lower detction limits #29

Merged
merged 17 commits into from
Nov 19, 2024
Merged

i28 generalise upper and lower detction limits #29

merged 17 commits into from
Nov 19, 2024

Conversation

hillalex
Copy link
Member

@hillalex hillalex commented Nov 1, 2024

Adds upper_detection_limit and lower_detection_limit arguments to the biokinetics class constructor, and takes the largest/smallest data values as defaults. These are used when scaling the data (if providing natural scale data), to determine which points are censored, and to centre the likelihoods for upper/lower censored data points.

Also adds truncate_upper and truncate_lower as boolean flags - if TRUE, this sets all points above/below the limits to those limits. Points at the limits are treated as censored.

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.53%. Comparing base (edeefbb) to head (ccaeb9c).
Report is 22 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
+ Coverage   93.47%   94.53%   +1.05%     
==========================================
  Files           9        9              
  Lines         506      567      +61     
==========================================
+ Hits          473      536      +63     
+ Misses         33       31       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@hillalex hillalex marked this pull request as ready for review November 4, 2024 15:29
R/plot.R Outdated
geom_smooth(aes(x = time_since_last_exp, y = value, colour = titre_type)) +
geom_point(aes(x = time_since_last_exp, y = value, colour = titre_type, shape = censored),
size = 0.5, alpha = 0.5) +
geom_smooth(data = subset(data, !censored),
Copy link
Member Author

@hillalex hillalex Nov 4, 2024

Choose a reason for hiding this comment

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

Added the censored indicator in to the input data plot as well, and excluding censored values from the spline - do we think this is the best way to deal with these data points? In the legacy data, the censored values are for some reason much lower than any other value in the dataset (so presumably lower than the actual assay detection limit), but if this wasn't the case would probably make sense to include them.

Copy link

Choose a reason for hiding this comment

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

Yeah I think so, might be worth changing the point marker to show a value is censored? Or add the lower and upper limits to the plot (just dashed lines?) I think should illustrate the point clearly

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah the shape is different if censored, but dashed lines could be good too 👍

dt_out <- data.table::copy(dt_in)
for (var in vars_to_transform) {
if (simplify_limits == TRUE) {
Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment am just warning if there are values outside of provided detection limits... but perhaps if there are, we should also keep this logic, i.e. set all higher values to the upper limit and all lower values to the lower limit?

Copy link

Choose a reason for hiding this comment

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

I think given the new warning this will work fine now right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, so we are now doing this when the data is first passed in, iff the strict_upper_limit/strict_lower_limit args are TRUE.

Copy link

@dchodge dchodge 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 Alex, all minor issues from me

R/plot.R Outdated
geom_smooth(aes(x = time_since_last_exp, y = value, colour = titre_type)) +
geom_point(aes(x = time_since_last_exp, y = value, colour = titre_type, shape = censored),
size = 0.5, alpha = 0.5) +
geom_smooth(data = subset(data, !censored),
Copy link

Choose a reason for hiding this comment

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

Yeah I think so, might be worth changing the point marker to show a value is censored? Or add the lower and upper limits to the plot (just dashed lines?) I think should illustrate the point clearly

Copy link

Choose a reason for hiding this comment

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

These are the plots I'm thinking of having the upper and lower limits on

R/biokinetics.R Outdated Show resolved Hide resolved
@hillalex hillalex requested a review from dchodge November 5, 2024 11:38
Copy link
Member Author

Choose a reason for hiding this comment

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

These look different because as well as adding upper/lower limits to the plot, I've made the points smaller and added a tmax argument with a default of 150 days post exposure

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still including censored points in the smoothing function, I thought that was probably the expected behaviour, but the artifically low values in the legacy data (below the real titre detection limit) does distort them.

@hillalex hillalex force-pushed the i28 branch 2 times, most recently from aa2cc63 to 27446ca Compare November 10, 2024 23:02
Copy link

@dchodge dchodge 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 now Alex!

values_above <- max_value > private$upper_censoring_limit
values_below <- min_value < private$lower_censoring_limit
if (strict_upper_limit) {
if (values_above) {
Copy link

Choose a reason for hiding this comment

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

Yeah this is much clearer terminology

dt_out <- data.table::copy(dt_in)
for (var in vars_to_transform) {
if (simplify_limits == TRUE) {
Copy link

Choose a reason for hiding this comment

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

I think given the new warning this will work fine now right?

@hillalex hillalex merged commit 45dab7e into main Nov 19, 2024
9 checks passed
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