Skip to content

Conversation

@dmendelowitz
Copy link
Contributor

Summary

Column headers of CSV files read as input for the MEF can now have any casing and will still be treated the same by the program

New behavior

The program should still run the same as before, but now CSV headers can have any casing (e.g. enrollmentStatus, ENROLLMENTSTATUS, and EnRoLlMeNtStAtUs are all valid and refer to the same thing).

Code changes

  • csvModule.js now converts all headers to lowercase upon reading the CSV and will convert any keys provided to a get function to lowercase to properly find values
  • Every single CSV extractor has had its version of a formatting function changed to accommodate lowercase csv headers
  • Every CSV extractor test file has had its fixtures and tests changed to expect lowercase headers from the csvModule
  • csvValidoator.js has been updated to convert the CSV schema and actual CSV headers to lowercase before comparing them

Testing guidance

  • Ensure that all previous functionality still works as expected
  • Mess up a few column headers with different casing and see that the extractor still works
  • Check that any change to some part of the program to accommodate case insensitivity hasn't messed up something else in some unforeseen way

Copy link
Contributor

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

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

This looks great! The changes are really minimal given that you had to change so much! I had a few small things inline and then I had a few other thoughts:

  • It looks like this line in CSVObservationExtractor.test.js should also be updated if you agree with the other similar comments.
  • It might be good to test that the CSV Validator can successfully validate headers that are in various casing that doesn't match the schema. I think that should cover the changes in csvValidator.js and CSVModule.js.
  • I think we'll need a small PR in the E-MEF because the ClinicalTrialExtractor there uses the CSV Module, so when we update the MEF, I think it will be looking for the lower case changes.

@jafeltra jafeltra self-assigned this Apr 22, 2021
@dmendelowitz
Copy link
Contributor Author

dmendelowitz commented Apr 23, 2021

Okay, I think I fixed the comments mentioned here, so give that another look. I also created a PR on the E-MEF (https://gitlab.mitre.org/mcode/epic-mcode-extraction-framework/-/merge_requests/128) for the Epic Clinical Trial Information extractor there that also uses CSVs

Copy link
Contributor

@julianxcarter julianxcarter left a comment

Choose a reason for hiding this comment

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

This looks great!

Copy link
Contributor

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for making those changes!

@jafeltra
Copy link
Contributor

I think this just needs a rebase to get back in line with develop, and then it should be good to merge!

@dmendelowitz dmendelowitz force-pushed the case-insensitive-csv-columns branch from 3debca4 to ca71c5e Compare April 26, 2021 17:42
@dmendelowitz dmendelowitz merged commit 8268a97 into develop Apr 26, 2021
@dmendelowitz dmendelowitz deleted the case-insensitive-csv-columns branch April 26, 2021 17:48
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.

4 participants