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

return kwargs from adapter.to_file funtions #456

Merged
merged 10 commits into from
Aug 3, 2023
Merged

return kwargs from adapter.to_file funtions #456

merged 10 commits into from
Aug 3, 2023

Conversation

savente93
Copy link
Contributor

Issue addressed

Fixes #182

Explanation

The description wasn't super clear about how this was supposed to be used but it was super quick to do, so I just took the liberty. For now the returned source_kwargs in the export function is unused, but hopefully, those (i.e. @DirkEilander) can use it now that it is available. This PR doesn't actually add any functionality, therefore I don't think tests or doc updates are necessary.

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.

Copy link
Contributor

@DirkEilander DirkEilander left a comment

Choose a reason for hiding this comment

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

This issue indeed wasn't clear at all. What I think we want to return in the to_file methods are any keyword arguments required to read back the data into an identical object. To read self-describing formats like netcdf and geotiff to a xarray.Dataset this is not necessary, but to properly read a csv back to a pandas.DataFrame you need to know the index column. I guess the required "return kwargs" can be specified per driver, but would need to try to be sure.

@savente93 savente93 marked this pull request as draft July 27, 2023 08:17
@savente93 savente93 marked this pull request as ready for review July 27, 2023 15:24
@savente93
Copy link
Contributor Author

For the CSV I've added the index_col to the kwargs that is returned, so I think it should be all good now. If we can think of any attributes needed later we can always add them

Copy link
Contributor

@DirkEilander DirkEilander left a comment

Choose a reason for hiding this comment

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

I added a small comment, let me know if you have questions.

hydromt/data_adapter/dataframe.py Outdated Show resolved Hide resolved
hydromt/data_adapter/dataframe.py Outdated Show resolved Hide resolved
@savente93
Copy link
Contributor Author

@DirkEilander Perhaps a tangential question, but I noticed that the export_data in the test_export_global_datasets raises the following warning: UserWarning: using the driver setting is deprecated. Please use vector_table instead. I'm wondering if this is something we can solve here now? We would check if the export we're doing is a csv based one but that feel really inelegant. What do you think?

@savente93
Copy link
Contributor Author

not to @savente93 :

  1. leave warnings, that's other PR
  2. update to reader_kwargs in other adapters
  3. add index_col = 0 to reader_kwargs in csv in dataframe and geodataframe

@savente93
Copy link
Contributor Author

@DirkEilander implemented what we discussed. Should be good to go now.

Copy link
Contributor

@DirkEilander DirkEilander left a comment

Choose a reason for hiding this comment

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

Made a small change to actually use the returned driver_kwargs.
Otherwise good to be merged!

@savente93 savente93 merged commit c1f0f6f into main Aug 3, 2023
7 checks passed
@savente93 savente93 deleted the return-kwargs branch August 3, 2023 16:24
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.

DataAdapter.to_file methods should return driver "kwargs"
2 participants