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.

Sign up for GitHub

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

Jump to bottom

ENH: Read CSV File Redesign #706

Merged
merged 37 commits into from
Oct 12, 2023
Merged

ENH: Read CSV File Redesign #706

merged 37 commits into from
Oct 12, 2023

Conversation

joeykleingers
Copy link
Contributor

@joeykleingers joeykleingers commented Sep 29, 2023 •
edited
Loading

  • Renamed the filter from ImportCSVData to ReadCSVFile, and also renamed all parameters, widgets, utilities, and helper classes.
  • Updated the parameter to only include the data required for the filter, removing unnecessary fields and make sure that all fields can be converted easily with the Python bindings.
  • Updated the filter to do additional error checking and cache the file's total lines and headers to avoid reading the entire file every preflight.
  • Moved the DynamicTableData tuple dimensions parameter into the CSVImporterData custom parameter.
  • Removed all references to "wizard", since the UI no longer involves using a wizard.
  • Updated the Python bindings to expose the new ReadCSVData struct to Python.
  • Updated the Read CSV File unit test and any other unit tests that use the filter.
  • Updated the regular filter documentation as well as the Python documentation.
  • Renamed ImportTextFilter to ReadTextDataArrayFilter, to avoid naming confusion.

@joeykleingers joeykleingers added the enhancement New feature or request label Sep 29, 2023
@joeykleingers joeykleingers self-assigned this Sep 29, 2023
@imikejackson imikejackson force-pushed the feature/ImportCSVDataRedesign branch 2 times, most recently from 2fa3ed4 to 23f663e Compare October 2, 2023 16:59
@joeykleingers joeykleingers force-pushed the feature/ImportCSVDataRedesign branch from efd214d to 142428e Compare October 4, 2023 15:21
@JDuffeyBQ
Copy link
Collaborator

It's probably a good idea to add a unit test for the blank line handling.

@joeykleingers joeykleingers force-pushed the feature/ImportCSVDataRedesign branch from 9c7a81f to 409862d Compare October 4, 2023 20:06
@joeykleingers
Copy link
Contributor Author

@JDuffeyBQ Blank lines test has been added. Approve unless there's anything else you'd like fixed/changed.

@joeykleingers
Copy link
Contributor Author

Actually, hang on... let me add one more test that checks to make sure that the filter handles unexpected inputs for the other arguments. Like if the user inputs 0 for the start row or a number larger than the total file size for the header row, for example.

@joeykleingers
Copy link
Contributor Author

@JDuffeyBQ Last unit test has been added. Ready for review again.

@joeykleingers joeykleingers force-pushed the feature/ImportCSVDataRedesign branch 2 times, most recently from cc2b209 to d2a89b4 Compare October 6, 2023 14:48
@imikejackson imikejackson force-pushed the feature/ImportCSVDataRedesign branch from 62c2722 to 7382a6f Compare October 6, 2023 22:35
@joeykleingers joeykleingers force-pushed the feature/ImportCSVDataRedesign branch from 1a3042d to 02f1276 Compare October 9, 2023 14:49
@joeykleingers joeykleingers changed the title Import CSV Data Redesign Import Text Data Redesign Oct 9, 2023
@joeykleingers joeykleingers changed the title Import Text Data Redesign ENH: Import Text Data Redesign Oct 9, 2023
@joeykleingers
Redesign CSV importer parameter data.
f381cc7 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Update unit tests.
14927f9 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Use Existing Attr Matrix tuple shape if option is chosen.
48d4c9c 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Rename files/variables to remove references to "wizard".
cab70ec 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Add preflight checks for the data types and skipped array mask sizes.
c3513aa 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Handle relative file paths.
5fe38cd 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Update the python bindings and the Import CSV python example.
292d961 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Update filter documentation.
65dbbd5 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Throw error for any empty lines, more checks, better error messages.
12ebeaf 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Remove unneeded dummy group creation step.
416d2f6 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Add blank lines test.
e40f600 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Add another unit test that tests the CSVImporterData values.
29ff6f1 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Remove unneeded unit test error codes.
9e4e638 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Update case numbers.
01e05eb 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Add preflight check that checks for 0 total tuples.
570fa26 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers
Update documentation.
a97c9da 
Signed-off-by: Joey Kleingers <[email protected]>
@joeykleingers joeykleingers force-pushed the feature/ImportCSVDataRedesign branch from 02f1276 to 4e8aaa2 Compare October 9, 2023 15:25
@imikejackson imikejackson force-pushed the feature/ImportCSVDataRedesign branch from 74d3f37 to 4a0d178 Compare October 9, 2023 17:11
@JDuffeyBQ
Copy link
Collaborator

I don't have a good suggestion but we have a similarly named filter in ImportTextFilter so I think that they should be easier to distinguish.

@imikejackson
Copy link
Contributor

These need renamed:

[ ] ImportTextFilter -> ReadTextDataArrayFilter
[ ] ImportTextDataFilter -> ReadCSVFileFilter

[ ] Rename the parameter from ImportTextDataParameter to ReadCSVFileParameter

[ ] TextImporterData::writeJson convert the delimiters to a std::vectorstd::string BEFORE writing
[ ] TextImporterData::readJson convert from std::vectorstd::string to std::vector after reading in from the JSON file.

@joeykleingers joeykleingers changed the title ENH: Import Text Data Redesign ENH: Read CSV File Redesign Oct 11, 2023
@joeykleingers
Copy link
Contributor Author

@imikejackson All of the most recent suggestions have been implemented. This is ready for final review and approval.

@imikejackson imikejackson force-pushed the feature/ImportCSVDataRedesign branch from 9ded331 to 30e5b6d Compare October 12, 2023 16:45
@imikejackson imikejackson enabled auto-merge (squash) October 12, 2023 16:51
@imikejackson imikejackson merged commit 583b073 into BlueQuartzSoftware:develop Oct 12, 2023
7 checks passed
@joeykleingers joeykleingers deleted the feature/ImportCSVDataRedesign branch October 12, 2023 19:17
imikejackson added a commit to imikejackson/simplnx that referenced this pull request Oct 20, 2024
@joeykleingers @imikejackson
ENH: Read CSV File Filter Redesign (BlueQuartzSoftware#706)
f6569c4 
* Redesign CSV importer parameter data.
* BUG FIX: Handle boolean types properly.
* Update ConvertTo<bool> logic.
* Update delimiter booleans to a vector of chars.
* Rename ImportCSVDataFilter to ReadCSVFileFilter.
* Rename ImportTextFilter to ReadTextDataArrayFilter.

---------

Signed-off-by: Joey Kleingers <[email protected]>
Signed-off-by: Michael Jackson <[email protected]>
Co-authored-by: Michael Jackson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imikejackson imikejackson imikejackson approved these changes

@JDuffeyBQ JDuffeyBQ Awaiting requested review from JDuffeyBQ

Assignees

@joeykleingers joeykleingers

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joeykleingers @JDuffeyBQ @imikejackson