Use less dtype inference when reading metadata into DataFrames

[victorlin]

Description of proposed changes

Support passing the dtype parameter to pandas.read_csv in augur.io.read_metadata, and use it to not infer data types in subcommands that don't need it.

Internal references to read_metadata that were not updated due to broad use of data types:

• export v1, v2: These support arbitrary columns for coloring. Data types should be inferred on all columns to ensure they are converted to JSON properly.

Related issue(s)

• Prompted by Set date column to be string #1235 (comment)

Testing

• Added a test for numerical queries that rely on dtypes
• Checks pass

Checklist

• Add a message in CHANGES.md summarizing the changes in this PR that are end user focused. Keep headers and formatting consistent with the rest of the file.

[codecov]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (e17bd07) 66.69% compared to head (2a90aab) 66.28%.
Report is 12 commits behind head on master.

❗ Current head 2a90aab differs from pull request most recent head 7e81765. Consider uploading reports for the commit 7e81765 to get more accurate results

Files	Patch %	Lines
augur/io/metadata.py	63.63%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1252      +/-   ##
==========================================
- Coverage   66.69%   66.28%   -0.42%     
==========================================
  Files          69       69              
  Lines        7321     7269      -52     
  Branches     1797     1784      -13     
==========================================
- Hits         4883     4818      -65     
- Misses       2170     2181      +11     
- Partials      268      270       +2     

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

[huddlej]

traits: This support arbitrary columns (via --columns) to perform discrete reconstruction on. I assume that numerical values as a numerical type might be useful here, so data types should be inferred on all columns.

I just ran into this issue where I need to run augur traits on values that are numeric (integers) but should be treated as strings (the integers are actually cluster labels, so they identify groups of samples and shouldn't be treated like numbers). Since augur traits only supports discrete trait analysis, I would expect no type inference from the metadata prior to running DTA. If we ever add support for continuous trait analysis, we would need to provide a way to explicitly enable type inference for columns. In the context of this specific command, that interface could look like --discrete-columns and --continuous-columns, if we wanted to support some combination of both inference modes in the same command.

From a slightly later comment on a different issue with the same topic, @victorlin noted that:

The general consensus was that we should use the "string" dtype for everything to avoid having to worry about how pandas automatically handles dtypes under the hood.

@victorlin @joverlee521 What do you think about revisiting the approach in this specific PR vs. some other approach?

[victorlin]

@huddlej thanks for letting me know about augur traits, I'll add it to this PR.

I need to update this with @joverlee521's suggestion to replace infer_dtypes with the more extensible dtype, then this should be ready for re-review.

[victorlin]

@huddlej added augur traits as 45983b2 and changelog entry:

traits: Previously, columns with only numeric values were treated as numerical data. These are now treated as categorical data for discrete trait analysis.

[victorlin]

This is implemented in #1294 as ac23e80

[huddlej]

Thanks, @victorlin! With the latest changes in this PR, augur traits does exactly what I would expect. I'm pleasantly surprised that switching the dtype to "string" throughout augur (except export) hasn't broken any subcommands that silently depended on pandas type inference.

For posterity, could you update the PR description to no longer mention augur traits as a module you omitted?

[victorlin]

I'm pleasantly surprised that switching the dtype to "string" throughout augur (except export) hasn't broken any subcommands that silently depended on pandas type inference.

I checked the subcommands to make sure that they don't depend on pandas type inference: e05038d...a65aa00

Looking back again, I'm not sure about frequencies. @huddlej since you authored 6548579, can you confirm that it's fine for all metadata to be str here? EDIT: asked on Slack

augur/augur/frequencies.py

Lines 117 to 120 in df9e8de

	# Annotate tips with metadata to enable filtering and weighting of
	# frequencies by metadata attributes.
	for key, value in metadata.loc[tip.name].items():
	tip.attr[key] = value

For posterity, could you update the PR description to no longer mention augur traits as a module you omitted?

Done!

[huddlej]

@victorlin Thanks for doublechecking about the use of metadata in the frequencies code. I can confirm that frequencies should not be affected by setting the default type to string. In addition to the date information that we parse from metadata, we also use metadata to weigh frequencies of individual records based on their values in a specific metadata column (e.g., region or country). We expect the values in the metadata and the weights provided by the user to be strings.

That said, because we could need a specific metadata field for this weighting process, this comment about only loading a subset of fields should probably mention that we'd need to optionally include the metadata column associated with the --weights-attribute value, when the user provides it.

[victorlin]

@huddlej thanks for the explanation!

I've updated the comment to:

# TODO: load only the ID, date, and --weights-attribute columns when
# read_metadata supports loading a subset of all columns.