TopoToolbox / pytopotoolbox

Python interface to TopoToolbox
https://topotoolbox.github.io/pytopotoolbox/
GNU General Public License v3.0
1 stars 3 forks source link

Provide example data #29

Closed Teschl closed 4 months ago

Teschl commented 4 months ago

In this pull request, I added the following functions:

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 commented 4 months ago

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 commented 4 months ago

https://github.com/TopoToolbox/DEMs

wschwanghart commented 4 months ago

Excellent. I think it's generally a good idea to host also the example DEMs on git although git is not really made for data hosting. However, so far it supports file sizes up to 50 Mb. We might also consider this here: https://docs.github.com/en/repositories/working-with-files/managing-large-files/about-git-large-file-storage

Am Di., 28. Mai 2024 um 19:57 Uhr schrieb William Kearney < @.***>:

https://github.com/TopoToolbox/DEMs

— Reply to this email directly, view it on GitHub https://github.com/TopoToolbox/pytopotoolbox/pull/29#issuecomment-2135823260, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4RU3DYTAE46OS3KWBL3GDZETASNAVCNFSM6AAAAABIM4D7ROVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZVHAZDGMRWGA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Teschl commented 4 months ago

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.