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

Provide example data #29

Merged
merged 8 commits into from
May 30, 2024
Merged

Provide example data #29

merged 8 commits into from
May 30, 2024

Conversation

Teschl
Copy link
Contributor

@Teschl Teschl commented May 28, 2024

In this pull request, I added the following functions:

  • topotoolbox.load_dem() enter DEM name as argument, it will download the tif if it isn't already cached.
  • topotoolbox.get_dem_names() will return a list of all available tifs for load_dem(). if we add a .txt which contains all names to the repository which contains the tifs, we can implement it more dynamically. (right now it's just a list in the .py file)
  • topotoolbox.read_tif() is a wrapper for creating a GridObject. Could be a cleaner way to create a GridObject inspired by pandas (pandas.read_csv creates a pandas.Dataframe).

Since the function get_save_location() in utils.py is not listed in __all__ it's not automatically imported when using the package. This way it's "private" and not usable for the user by default.

I also fixed some line length issues in the mixin files for the GridObject. The maximum line length is 79 chars.

@Teschl
Copy link
Contributor Author

Teschl commented May 28, 2024

Regarding the repository that contains the examples, right now, it's the https://github.com/wschwanghart/DEMs repository. So we might want to fork it to https://github.com/TopoToolbox/DEMs. Then we also should add a file like contents.txt in which we store all names of the provided DEMs. That means we could remove the hardcoded DEM_NAMES of utils.py and generate them dynamically.

If implemented, this should resolve #9 .

@wkearn
Copy link
Contributor

wkearn commented May 28, 2024

Copy link
Contributor

@wkearn wkearn left a comment

Choose a reason for hiding this comment

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

Looks pretty good, @Teschl. This should be sufficient for our testing and documentation purposes at the moment. I suggested a few changes that make sense right now.

In the future, we might want to think about other ways that users might want to access data and how we can accomodate them. The MATLAB TopoToolbox provides, for example, a way to access the OpenTopography API that could be nice to have across our packages.

src/topotoolbox/utils.py Show resolved Hide resolved
src/topotoolbox/utils.py Outdated Show resolved Hide resolved
src/topotoolbox/utils.py Show resolved Hide resolved
src/topotoolbox/utils.py Show resolved Hide resolved
@wschwanghart
Copy link

wschwanghart commented May 29, 2024 via email

@Teschl
Copy link
Contributor Author

Teschl commented May 29, 2024

I have addressed the more bug-fix related requested changes.

Would it make sense to read the data through rasterio in here and then use read_tif in the GridObject constructor? That might let us use the GridObject constructor to dispatch to different methods for loading data depending on the arguments provided to it. That might be a problem for another pull request though.

and maybe adding store_dem() can be handled in a different pull request.

@wkearn wkearn merged commit 3eb8997 into TopoToolbox:main May 30, 2024
3 checks passed
@Teschl Teschl deleted the load-examples branch May 30, 2024 13:44
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.

3 participants