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

tdb/elife_upload: Update assay_date parsing #146

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

joverlee521
Copy link
Contributor

Description of proposed changes

Updates to the assay date parsing in tdb/elife_upload:

  • make parsing of assay date from fstem a little more robust
  • use the fstem's assay date as backup to the date that is in the measurement records.

Motivated by the latest discussion on ingesting the VIDRL flat files.

Checklist

  • Checks pass

Pull out assay_date parsing from filename into a separate method
`parse_assay_date_from_filename`.

This is done in preparation to update the assay date parsing for VIDRL
flat files.
The assay date is the same for all measurement records from the same
fstem, so only parse the assay date once.
Use regex to find all matches for the expected date format 'YYYYMMDD'.
Then use the datetime module to validate the date string and check that
the date is earlier than the date we are parsing the file.

I made the decision to use the latest date if there are multiple matches
with the expectation that we would be parsing files not long after the
assay date.
Only fill in with the assay date parsed from the fstem when the
measurment does not have an assay date. This is done in preparation for
parsing the flat VIDRL files which will include assay date as a column.¹

This ensures that the fstem date is only a backup to the date that is
set within `format_date`.²

¹ https://bedfordlab.slack.com/archives/C03KWDET9/p1699914235686809
² https://github.com/nextstrain/fauna/blob/8088646ce0ba438310cdc9f919080950d0767c46/tdb/upload.py#L328
Copy link
Contributor

@huddlej huddlej left a comment

Choose a reason for hiding this comment

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

This is so much cleaner, @joverlee521! I had a question about the third logical case in parsing assay dates from file names, but I don't think it's blocking.

Comment on lines 99 to 102
sorted_dates = list(map(lambda x: datetime.datetime.strftime(x, '%Y-%m-%d'), sorted(valid_dates)))
assay_date = sorted_dates[-1]
print(f"Found multiple potential assay dates in filename {fstem!r}: {sorted_dates}.",
f"Using the last valid date as the assay date: {assay_date!r}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this case ever happen? Are we sure that we want to pick the latest date as the assay date when we do find multiple dates or do we want to throw an error so the user has a chance to fix the file name before the upload happens?

As a minor aesthetic note, you could also sort the dates with a list comprehension to avoid using list, map, and lambda:

sorted_dates = [datetime.datetime.strftime(valid_date, '%Y-%m-%d') for valid_date in sorted(valid_dates)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this case ever happen? Are we sure that we want to pick the latest date as the assay date when we do find multiple dates or do we want to throw an error so the user has a chance to fix the file name before the upload happens?

Not yet 😄 You're right that it would probably be better to throw an error here and manually fix the filename. I was thinking we would want the latest date if the file had an updated date appended, but better to be explicit about having one date in the filename.

It's unclear if we'll ever run into the case where there are multiple
assay dates in the filename, but if we do, raise an exception to alert
the user to manually fix the filename before upload.
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