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

Add support for multiple csv segmented by var #489

Merged
merged 19 commits into from
Oct 6, 2023
Merged

Conversation

savente93
Copy link
Contributor

@savente93 savente93 commented Aug 28, 2023

Issue addressed

#453

Explanation

So I don't think this PR addresses an issue exactly, but it was something that I setup in #486 and wanted to finish anyway. I think it makes significant progress if not outright solve #453 (not totally sure which of the two it is, I'll leave that for the reviewer decide, or we can discuss it during stadnup/refinement) and makes some additional progress towards #372.

This simply adds the possibility for open_mfcsv to not only parse files per id, but also files per variables containing all ids (though not at the same time). The resulting dataset is the same as when the same data was segmented by id. I think it's a nice flexibility to have.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Tests & pre-commit hooks pass
  • Updated documentation if needed
  • Updated changelog.rst if needed

Additional Notes (optional)

Add any additional notes or information that may be helpful.

@savente93
Copy link
Contributor Author

Makrking this as ready for review as the failure in the CI seems to happen after all the tests have passed. Possibly it's somehthing with the report generation. I'm investigating that seperately.

@savente93 savente93 marked this pull request as ready for review August 28, 2023 11:38
@savente93
Copy link
Contributor Author

For posterities sake, the CIs crach with the following message:

---------- coverage: platform linux, python 3.9.17-final-0 -----------
Coverage XML written to file coverage.xml

================= 143 passed, 65 warnings in 283.67s (0:04:43) =================
double free or corruption (!prev)
/home/runner/work/_temp/cb436074-82f6-48ae-bf0a-9afa0e4f349b.sh: line 1:  2714 Aborted                 (core dumped) python -m pytest --verbose --cov=hydromt --cov-report xml

@savente93
Copy link
Contributor Author

note to self: add to doc index

Copy link
Contributor

@Tjalling-dejong Tjalling-dejong left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@savente93 savente93 merged commit 0236918 into main Oct 6, 2023
7 checks passed
@savente93 savente93 deleted the multipe-csv-per-var branch October 6, 2023 08:16
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.

EHN: Support multiple timeseries files for creating a GeoDataset object
3 participants