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

Added pathlib support and resolved conflicts. #1731

Closed
wants to merge 0 commits into from

Conversation

pioneerHitesh
Copy link
Contributor

added pathlib support , removed checked_instance_type function

@github-actions github-actions bot added datasets Geospatial or benchmark datasets testing Continuous integration testing labels Nov 18, 2023
@adamjstewart
Copy link
Collaborator

Let's wait for a response on pytorch/vision#8120 before we do too much. We might be able to get them to support pathlib in the next release.

@pioneerHitesh
Copy link
Contributor Author

pioneerHitesh commented Nov 18, 2023

Let's wait for a response on pytorch/vision#8120 before we do too much. We might be able to get them to support pathlib in the next release.

@adamjstewart may you please check your LinkedIn.

@adamjstewart adamjstewart marked this pull request as draft November 18, 2023 15:33
@adamjstewart
Copy link
Collaborator

Based on the current state of pytorch/vision#8120, I think the easiest thing to do for now would be to define functions in torchgeo.datasets.utils that are simply wrappers around the torchvision functions. These functions would accept both str and pathlib, then we would cast to str for torchvision. Then we wouldn't need to cast in every single dataset file. Thoughts?

@pioneerHitesh
Copy link
Contributor Author

pioneerHitesh commented Jan 20, 2024

Based on the current state of pytorch/vision#8120, I think the easiest thing to do for now would be to define functions in torchgeo.datasets.utils that are simply wrappers around the torchvision functions. These functions would accept both str and pathlib, then we would cast to str for torchvision. Then we wouldn't need to cast in every single dataset file. Thoughts?

@adamjstewart It seems like they have added support for pathlib in datasets/utils.py. The following PR is for the same. So we may not need to define extra functions for casting pathlib to str. Let me know if we should start work or wait some more time?

@adamjstewart
Copy link
Collaborator

Fantastic, let's wait until that makes it into a stable release and then rebase this PR.

@adamjstewart
Copy link
Collaborator

I believe torchvision 0.18 includes full pathlib support. If you're still interested, now would be a good time to try to revive this. It's probably easiest to create a new PR since most files have changed since last year.

As a first pass, can you change the type hints (without changing the code or type casting) just to see how much of pathlib we already support? Torchvision only supports str | Path[str], so let's do the same in TorchGeo.

@pioneerHitesh
Copy link
Contributor Author

@adamjstewart That's great, but do i have to create a new PR?

@adamjstewart
Copy link
Collaborator

You don't have to, although it's easier to create a new PR than to resolve all of the merge conflicts.

@pioneerHitesh
Copy link
Contributor Author

@adamjstewart There are no stubs available for some modules, so mypy is not able to perform type checking resulting in [import-untyped] errors. Should i just suppress those errors or anything else needs to be done?

@adamjstewart
Copy link
Collaborator

Can you push your changes to GitHub so I can see what you mean? I don't fully understand.

@pioneerHitesh
Copy link
Contributor Author

@adamjstewart I haven't made any changes , on cloning the repo and running mypy * gives error: Skipping analyzing "rasterio.crs": module is installed, but missing library stubs or py.typed marker [import-untyped] in the datasets folder.

@adamjstewart
Copy link
Collaborator

In order for mypy to find the settings in pyproject.toml that ignore the errors you're describing, I think you have to run mypy from the same directory that contains pyproject.toml. So don't run mypy * in the datasets folder, run mypy . in the root directory. Hopefully that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datasets Geospatial or benchmark datasets testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants