Port `transform-field-names` to `augur curate rename` #1506

jameshadfield · 2024-07-01T04:30:26Z

See commit messages for details, and especially the added tests which describe the expected behaviour - most of which is slightly different from the previous transform-field-names script.

This assumes that NDJSON records have the same fields, so it's probably blocking on #1510, although I can add a simple check for that within the rename code in the name of expediency if desired.

Closes #1484

tests/functional/curate/cram/rename/general.t

See discussion in PR review for context <#1506 (comment)>

augur/curate/rename.py

genehack · 2024-07-02T16:42:33Z

augur/curate/rename.py

+
+    field_map = []
+    for field in field_map_arg:
+        old_name, new_name = field.split('=')


worth stripping whitespace off old_name and new_name here?

I had originally glossed over this thinking any sane IFS would take care of it for me, but of course it won't do this when the arguments are quoted. Fixed up in force push and a bunch of tests added here, including checking that we have one and only one "=" character.

augur/curate/rename.py

tests/functional/curate/cram/rename/general.t

Copied from <https://github.com/nextstrain/ingest/blob/c94d78d1f38b99e893007a76526f3d3824ecded0/transform-strain-names> which was itself copied from <https://github.com/nextstrain/mpox/blob/5969604dfe426745b789746427b580c69d484790/ingest/bin/transform-strain-names> Only change in this commit was to remove the shebang and not make the file executable.

Includes changes to make the copied script work as a new Augur subcommand and fit in with the codebase. The functional behaviour of the actual renaming is unchanged, although the I/O options are now expanded as we inherit the general `augur curate` machinery.

See discussion in PR review for context <#1506 (comment)>

jameshadfield · 2024-07-04T00:22:13Z

@joverlee521 I think this is good to go now. I added 650bd56 which allows you to drop columns via --field-map X= as well.

See discussion in PR review for context <#1506 (comment)>

Tests describe desired output, not current output

to match expected behaviour in tests. The main changes functional changes are around the order of fields, where we now rename "in-place" rather than adding the renamed column at the end (which for TSV output is the last column). More sanity checks are performed on arguments and they are cross-referenced with the provided records. Note that this relies on each record having the same fields, and this is not asserted here. See <#1510>

See discussion in PR review for context <#1506 (comment)>

codecov · 2024-07-04T01:28:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.90%. Comparing base (b69444a) to head (0d0401b).
Report is 222 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1506      +/-   ##
==========================================
+ Coverage   69.69%   69.90%   +0.21%     
==========================================
  Files          74       75       +1     
  Lines        7827     7882      +55     
  Branches     1914     1933      +19     
==========================================
+ Hits         5455     5510      +55     
  Misses       2086     2086              
  Partials      286      286

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

joverlee521 · 2024-07-08T22:41:31Z

augur/curate/rename.py

+        if not new_name and not force:
+            raise AugurError(f"The field-map {field!r} doesn't specify a name for the new field."
+                              " If you mean to drop this field you must specify --force.")


I'm slightly worried that this is conflating the --field-map/--force flag.

Thoughts on adding an explicit --drop-fields column?

To be clear, is the suggestion here to:

drop the --force flag, and

make --field-map require both old and new names (and throw an error if new name is missing), and

add an explicit --drop-fields option that does what it says on the tin?

If I've got that right, I think it's a sound suggestion — it's more explicit around what it's doing, and more bullet-proof against something like an accidental botched config edit.

I'm new to the curate world so I won't push against this. How about we just drop 650bd56 which will remove the drop functionality and shift this to a new issue. The resulting situation will require that --field-map has non-empty new and old fields (see tests). Note that --force is still used for the situation where we overwrite an existing column name (see tests).

How about we just drop 650bd56 which will remove the drop functionality and shift this to a new issue.

Sounds good to me. I can write up a separate issue.

Commit dropped in force-push

Proposed drop column function in #1526

Following instructions in `DEV_DOCS.md` and the (forthcoming) PR <https://github.com/nextstrain/augur/pull/1527/files>. These docs were missed during development of the rename command <#1506> as that predated the CI checks added in <#1525>

jameshadfield commented Jul 1, 2024

View reviewed changes

tests/functional/curate/cram/rename/general.t Outdated Show resolved Hide resolved

joverlee521 mentioned this pull request Jul 1, 2024

augur curate I/O: verify NDJSON records have the same fields #1510

Closed

jameshadfield force-pushed the james/curate-rename branch 2 times, most recently from 47e03b9 to 3942969 Compare July 1, 2024 23:15

jameshadfield marked this pull request as ready for review July 1, 2024 23:15

jameshadfield force-pushed the james/curate-rename branch from 3942969 to a3615ee Compare July 1, 2024 23:17

jameshadfield requested a review from a team July 1, 2024 23:17

jameshadfield force-pushed the james/curate-rename branch from a3615ee to 1671cdb Compare July 1, 2024 23:23

jameshadfield commented Jul 1, 2024

View reviewed changes

tests/functional/curate/cram/rename/general.t Outdated Show resolved Hide resolved

jameshadfield force-pushed the james/curate-rename branch 2 times, most recently from 9193774 to b81f747 Compare July 1, 2024 23:39

jameshadfield commented Jul 1, 2024

View reviewed changes

tests/functional/curate/cram/rename/general.t Outdated Show resolved Hide resolved

jameshadfield added a commit that referenced this pull request Jul 2, 2024

[curate rename] allow column duplication

0a2096d

See discussion in PR review for context <#1506 (comment)>

jameshadfield force-pushed the james/curate-rename branch from b81f747 to 0a2096d Compare July 2, 2024 00:59

jameshadfield added a commit that referenced this pull request Jul 2, 2024

[curate rename] allow column duplication

3e9246e

See discussion in PR review for context <#1506 (comment)>

jameshadfield force-pushed the james/curate-rename branch from 0a2096d to 3e9246e Compare July 2, 2024 01:00

jameshadfield requested a review from joverlee521 July 2, 2024 01:19

genehack approved these changes Jul 2, 2024

View reviewed changes

joverlee521 reviewed Jul 2, 2024

View reviewed changes

tests/functional/curate/cram/rename/general.t Outdated Show resolved Hide resolved

victorlin mentioned this pull request Jul 2, 2024

Improve typing experience with CI #1516

Closed

joverlee521 mentioned this pull request Jul 2, 2024

Improve help output for augur curate subcommands #1521

Open

jameshadfield added 2 commits July 4, 2024 12:14

jameshadfield added a commit that referenced this pull request Jul 4, 2024

[curate rename] allow column duplication

8afabe4

See discussion in PR review for context <#1506 (comment)>

jameshadfield force-pushed the james/curate-rename branch from 3e9246e to 0357705 Compare July 4, 2024 00:15

jameshadfield added a commit that referenced this pull request Jul 4, 2024

[curate rename] allow column duplication

0930b9e

See discussion in PR review for context <#1506 (comment)>

jameshadfield force-pushed the james/curate-rename branch from 0357705 to 134f384 Compare July 4, 2024 01:03

jameshadfield added 2 commits July 4, 2024 13:15

[curate rename] Add tests

c2b2297

Tests describe desired output, not current output

[curate rename] allow column duplication

8c3ac16

See discussion in PR review for context <#1506 (comment)>

jameshadfield force-pushed the james/curate-rename branch from 134f384 to c7091ce Compare July 4, 2024 01:16

genehack approved these changes Jul 8, 2024

View reviewed changes

joverlee521 reviewed Jul 8, 2024

View reviewed changes

changelog

0d0401b

jameshadfield force-pushed the james/curate-rename branch from c7091ce to 0d0401b Compare July 8, 2024 23:11

joverlee521 approved these changes Jul 8, 2024

View reviewed changes

joverlee521 mentioned this pull request Jul 8, 2024

augur curate rename: Add option to drop fields/columns #1526

Open

jameshadfield merged commit 18e07b4 into master Jul 9, 2024
20 checks passed

jameshadfield deleted the james/curate-rename branch July 9, 2024 05:27

jameshadfield mentioned this pull request Jul 10, 2024

Update docs for (new) augur curate rename #1528

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port `transform-field-names` to `augur curate rename` #1506

Port `transform-field-names` to `augur curate rename` #1506

jameshadfield commented Jul 1, 2024 •

edited

Loading

genehack Jul 2, 2024

jameshadfield Jul 4, 2024 •

edited

Loading

jameshadfield commented Jul 4, 2024 •

edited

Loading

codecov bot commented Jul 4, 2024 •

edited

Loading

joverlee521 Jul 8, 2024

genehack Jul 8, 2024

jameshadfield Jul 8, 2024

joverlee521 Jul 8, 2024

jameshadfield Jul 8, 2024

joverlee521 Jul 8, 2024

Port transform-field-names to augur curate rename #1506

Port transform-field-names to augur curate rename #1506

Conversation

jameshadfield commented Jul 1, 2024 • edited Loading

genehack Jul 2, 2024

Choose a reason for hiding this comment

jameshadfield Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

jameshadfield commented Jul 4, 2024 • edited Loading

codecov bot commented Jul 4, 2024 • edited Loading

Codecov Report

joverlee521 Jul 8, 2024

Choose a reason for hiding this comment

genehack Jul 8, 2024

Choose a reason for hiding this comment

jameshadfield Jul 8, 2024

Choose a reason for hiding this comment

joverlee521 Jul 8, 2024

Choose a reason for hiding this comment

jameshadfield Jul 8, 2024

Choose a reason for hiding this comment

joverlee521 Jul 8, 2024

Choose a reason for hiding this comment

Port `transform-field-names` to `augur curate rename` #1506

Port `transform-field-names` to `augur curate rename` #1506

jameshadfield commented Jul 1, 2024 •

edited

Loading

jameshadfield Jul 4, 2024 •

edited

Loading

jameshadfield commented Jul 4, 2024 •

edited

Loading

codecov bot commented Jul 4, 2024 •

edited

Loading