-
Notifications
You must be signed in to change notification settings - Fork 18
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
Issue 11 #20
Issue 11 #20
Conversation
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.
@nbokulich, generally the tests would go in at the same time as the new functionality. Are these in another PR right now?
q2_intervention/plugin_setup.py
Outdated
'either state_1 or state_2, that subject will be dropped and ' | ||
'reported in standard output by default. Set duplicates="ignore" ' | ||
'reported in standard output by default. Set replicates="ignore" ' |
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.
should this be random
instead of ignore
?
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.
👍
q2_intervention/_intervention.py
Outdated
|
||
|
||
def paired_differences(output_dir: str, metadata: qiime2.Metadata, | ||
group_column: str, metric: str, state_column: str, | ||
state_1: str, state_2: str, individual_id_column: str, | ||
parametric: bool=False, palette: str='Set1', | ||
drop_duplicates: bool=True, table: pd.DataFrame=None | ||
drop_replicates: str='error', table: pd.DataFrame=None |
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 don't think this should be called drop_replicates
- maybe replicate_handling
?
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.
👍
q2_intervention/_utilities.py
Outdated
@@ -27,28 +27,40 @@ | |||
TEMPLATES = pkg_resources.resource_filename('q2_intervention', 'assets') | |||
|
|||
|
|||
def _check_inputs(state_1, state_2): |
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.
Could you rename this, maybe _validate_state_values
, to be a little more self-documenting?
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 intend to validate more inputs in a future PR so will use _validate_input_values
instead of _validate_state_values
thanks @gregcaporaso ! I added a unit test and revised per your suggestions. |
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 @nbokulich, a couple of minor things...
results = [] | ||
group_members = df[group_column] == group_value | ||
group_md = df[group_members] | ||
for individual_id in set(group_md[individual_id_column]): | ||
result = [] | ||
for state_value in state_values: | ||
state_value = df[state_column].dtype.type(state_value) | ||
individual_id = \ | ||
df[individual_id_column].dtype.type(individual_id) | ||
individual_id = df[individual_id_column].dtype.type(individual_id) | ||
_state = df[state_column] == state_value | ||
_ind = df[individual_id_column] == individual_id | ||
individual_at_state_idx = group_md[_state & _ind].index | ||
if len(individual_at_state_idx) > 1: | ||
print("Multiple values for {0} {1} at {2} {3} ({4})".format( |
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.
These print
statements aren't going to be very useful here, since with some interfaces users will see them, and with others they won't. In general, print
statements shouldn't be in any QIIME actions. Instead, these types of warnings should be included in the visualization - for example, see the yellow warning box in this viz.
Since I know you don't have a lot of time to work on this before you leave, could you create an issue to replace these print
statements with warnings in the visualization?
q2_longitudinal/_utilities.py
Outdated
break | ||
else: | ||
elif replicate_handling == 'random': | ||
individual_at_state_idx = [choice(individual_at_state_idx)] | ||
elif len(individual_at_state_idx) == 0: |
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.
Instead of ending this with break
, could you replace break
with pass
, and then put result.append(individual_at_state_idx[0])
in an else
block?
q2_longitudinal/plugin_setup.py
Outdated
'detected, "error" causes method to fail; "drop" will discard all ' | ||
'subject IDs with replicate samples at either state_1 or state_2; ' | ||
'"random" chooses one representative at random from among ' | ||
'replicates; "mean" and "median" compute average values across ' |
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.
mean
and median
aren't implemented, right? Can you remove those from this description?
thanks @gregcaporaso ! The latter two changes are made as suggested. The first change (about warning messages) has been turned into ticket #23 |
fixes #11
This does everything described in #11 with the exception of calculating mean and median values when replicates are found. This is a bit too complicated to implement at this stage for two reasons: (1) at this stage we are just grabbing the IDs to compare, and the pairwise difference/distance values are computed later; (2) this would behave very differently for each method — e.g., average alpha diversity is easy but how do we calculate average distance? If you think we should still support averaging replicates in a later release, we can leave this issue open or create a dedicated issue.