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

feat: OSM Tile Loader #217

Merged
merged 42 commits into from
Apr 27, 2023
Merged

feat: OSM Tile Loader #217

merged 42 commits into from
Apr 27, 2023

Conversation

mprzymus
Copy link
Contributor

Created loader which downloads tiles for specified area.

Copy link
Collaborator

@piotrgramacki piotrgramacki left a comment

Choose a reason for hiding this comment

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

Nice work, thanks ;) I left some comments, mostly regarding some code standards.
Unfortunately, there is one error, which made it impossible for me to run this code

examples/loaders/osm_tile_loader.ipynb Show resolved Hide resolved
srai/loaders/osm_loaders/osm_tile_loader.py Outdated Show resolved Hide resolved
srai/loaders/osm_loaders/osm_tile_data_collector.py Outdated Show resolved Hide resolved
srai/regionizers/slippy_map_regionizer.py Outdated Show resolved Hide resolved
srai/loaders/osm_loaders/osm_tile_data_collector.py Outdated Show resolved Hide resolved
srai/loaders/osm_loaders/osm_tile_data_collector.py Outdated Show resolved Hide resolved
srai/regionizers/slippy_map_regionizer.py Outdated Show resolved Hide resolved
srai/regionizers/slippy_map_regionizer.py Show resolved Hide resolved
srai/loaders/osm_loaders/osm_tile_loader.py Outdated Show resolved Hide resolved
@Calychas Calychas changed the title Osm tile loader feat: OSM Tile Loader Apr 2, 2023
Copy link
Collaborator

@Calychas Calychas left a comment

Choose a reason for hiding this comment

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

Great work @mprzymus!

I'm nitpicking, but I see that commits are not following the conventional commit message. I'm not sure if pre-commit ran correctly locally on your side.
Please refer to CONTRIBUTING.md and install pre-commit correctlly. No need to fix the messages now (in the end they are irrelevant as the PR is squashed), but for future reference ;)

@Calychas Calychas linked an issue Apr 3, 2023 that may be closed by this pull request
@Calychas Calychas added the enhancement New feature or request label Apr 3, 2023
@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #217 (630da0e) into main (4d01a99) will increase coverage by 0.00%.
The diff coverage is 97.65%.

@@           Coverage Diff            @@
##             main     #217    +/-   ##
========================================
  Coverage   97.54%   97.54%            
========================================
  Files          52       55     +3     
  Lines        1506     1632   +126     
========================================
+ Hits         1469     1592   +123     
- Misses         37       40     +3     
Flag Coverage Δ
macos-latest-python3.10 97.54% <97.65%> (+<0.01%) ⬆️
ubuntu-latest-python3.10 97.48% <97.65%> (-0.06%) ⬇️
ubuntu-latest-python3.8 97.48% <97.65%> (+0.01%) ⬆️
ubuntu-latest-python3.9 97.48% <97.65%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
srai/loaders/osm_loaders/osm_tile_loader.py 92.85% <92.85%> (ø)
srai/loaders/__init__.py 100.00% <100.00%> (ø)
srai/loaders/osm_loaders/__init__.py 100.00% <100.00%> (ø)
...rai/loaders/osm_loaders/osm_tile_data_collector.py 100.00% <100.00%> (ø)
srai/regionizers/__init__.py 100.00% <100.00%> (ø)
srai/regionizers/slippy_map_regionizer.py 100.00% <100.00%> (ø)

Copy link
Collaborator

@RaczeQ RaczeQ left a comment

Choose a reason for hiding this comment

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

Really nice job! I'm delighted that you could stick to the rest of the codebase style.
I've pointed out some things that irked me though.

The next good addition would be automatic stitching of the downloaded tiles and cropping them to the given region shape or at least the bounding box of it (some regionizer/feature generator?)

srai/loaders/osm_loaders/osm_tile_loader.py Outdated Show resolved Hide resolved
srai/regionizers/slippy_map_regionizer.py Outdated Show resolved Hide resolved
srai/regionizers/slippy_map_regionizer.py Outdated Show resolved Hide resolved
srai/regionizers/slippy_map_regionizer.py Outdated Show resolved Hide resolved
srai/regionizers/slippy_map_regionizer.py Outdated Show resolved Hide resolved
srai/loaders/osm_loaders/osm_tile_loader.py Outdated Show resolved Hide resolved
srai/loaders/osm_loaders/osm_tile_data_collector.py Outdated Show resolved Hide resolved
srai/loaders/osm_loaders/osm_tile_data_collector.py Outdated Show resolved Hide resolved
srai/loaders/osm_loaders/osm_tile_data_collector.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Calychas Calychas left a comment

Choose a reason for hiding this comment

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

Left some minor comments, but overall outstanding work Marcin!

examples/regionizers/slippy_map_regionizer.ipynb Outdated Show resolved Hide resolved
srai/regionizers/slippy_map_regionizer.py Outdated Show resolved Hide resolved
Calychas
Calychas previously approved these changes Apr 26, 2023
Copy link
Collaborator

@Calychas Calychas left a comment

Choose a reason for hiding this comment

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

LGTM! But wait for others before merging

pyproject.toml Outdated Show resolved Hide resolved
srai/loaders/osm_loaders/osm_tile_loader.py Outdated Show resolved Hide resolved
@Calychas Calychas merged commit 94a3a6b into kraina-ai:main Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support raster tiles
4 participants