-
Notifications
You must be signed in to change notification settings - Fork 2
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
Unscheduled visits #47
Conversation
@@ -94,16 +103,9 @@ export const controlInputs = [ | |||
}, | |||
{ type: 'number', label: 'Lower Limit', option: 'y.domain[0]', require: true }, | |||
{ type: 'number', label: 'Upper Limit', option: 'y.domain[1]', require: true }, | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we confirmed with Jack that this is ok to remove? Seems like it would be easy enough to keep. Maybe just refactor using the same workflow as removeUnscheduledVisits.js
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On further thought, I think the "hide visits with no data" control is worth keeping unless there is a compelling reason not to do so ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, on second thought it's nice to have.
@samussiah #43 needs test notes. Do we need a new data set with simulated unscheduled visits? |
src/defaultSettings.js
Outdated
@@ -20,6 +20,8 @@ const defaultSettings = { | |||
boxplots: true, | |||
violins: false, | |||
missingValues: ['', 'NA', 'N/A'], | |||
unscheduled_visits: false, | |||
unscheduled_visit_pattern: /unscheduled|early termination/i, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably lean towards making this text based instead of requiring users to understand regex. Or I suppose we could support both by adding unscheduled_visit_terms:["unscheduled","early termination"]
alongside the unscheduled_visit_pattern
and then building some logic regarding which one takes precedence ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is unscheduled visits typically have a suffix that indicates where the visit fell in the schedule of events. I've added another parameter, unscheduled_visit_values
that default to null
but overrides the unscheduled_visit_pattern
if specified.
…ve records rather than measures
…s precedence over unscheduled_visit_pattern
data/safetyData/ADBDS.csv has been updated with unscheduled visits and missing values. Ready for your review @jwildfire . |
src/onInit/addVariables.js
Outdated
d.unscheduled = this.config.unscheduled_visit_pattern.test( | ||
d[this.config.time_settings.value_col] | ||
); | ||
d.unscheduled = this.config.unscheduled_visit_values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@@ -230,7 +230,9 @@ | |||
boxplots: true, | |||
violins: false, | |||
missingValues: ['', 'NA', 'N/A'], | |||
visits_without_data: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd vote for unscheduled visits
to be true
by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind - I'd didn't notice the control. I think it's looking good as is.
Passed Code Review. Passed to David for Feature Testing |
The tests notes passed; great work Spencer! |
Issues
Closes #43