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

refactor SEPAL map #463

Merged
merged 45 commits into from
May 31, 2022
Merged

refactor SEPAL map #463

merged 45 commits into from
May 31, 2022

Conversation

12rambau
Copy link
Member

@12rambau 12rambau commented May 4, 2022

Fix #422
Fix #456

Main

The main objectives of this PR are to remove all dependencies to geemap as the number of functionalities provided is growing out of control and we are not using 10% of them. Also I took the occasion to partially revamp everything that was in mapping

new_map

To test the new code, you can use the dataset I provide in the following GIST: https://gist.github.com/12rambau/852d82c3b2c53ceee4a05c0f4cf01d9a

detail

  • no more dependencies to geemap (it goees a bit faster but nothing that sensible)
  • split basemaps from the other (the have the base member to true) there are now display in a different section of the layer

Capture d’écran 2022-05-23 à 19 15 21

  • create a handy find_layermethod it accept every possible key: layer, layer_name, index. return None if nothing is found
  • since we have find_layer I overwritten remove_layer to use the same parameter (index, layer, layer_name). raise an error if nothing is found
  • remove_all long wanting (at least from me) method to remove all layer from the map (leaving the basemaps).
  • extract the DrawControl from the SepalMap. it will be way easier to solve customize the drawControl object to manage edition (not natively set up) #386 from there
  • clean the way we store layers (remove the typed list local_layerand ee_layer as they are useless
  • clean and deprecated the unused methods, 4 of them should be fully removed in 3.0
  • create the EELayer to store the original dataset used in the layers (mainly for the v_inspector)
  • store the raster path in the LocaleTileLayer (same reasons)
  • extract the v_inspector from SepalMap and revamped the whole thing (I think it's nicer, and it's able to read the 4 types of data)
  • create a MapBtn that use the appropriate styling params to be responsive and nicely display on the map. as it's a vuetify element we can use icons from latest fas/mdi. I changed both the v_inspector and the fullscreen to use it. I'll handle the issue of the tooltip in another PR.
  • get rid of the small dot that was appearing sometime on the Map

TODO

  • use messages for the labels
  • test everything
  • add the new components to the documentation

conclusion

I don't want to add new things (tooltips, edit in drawcontrol, fullscreen bug) to this PR as it would make it way too big. I'm happy to receive feedbacks on the new component to correct them

Some of the libs start to be deprecating python 3.6 I think we'll drop the support starting from this PR

@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

Merging #463 (dc74419) into master (2f49625) will increase coverage by 1.26%.
The diff coverage is 91.51%.

❗ Current head dc74419 differs from pull request most recent head 6ed17a2. Consider uploading reports for the commit 6ed17a2 to get more accurate results

@@            Coverage Diff             @@
##           master     #463      +/-   ##
==========================================
+ Coverage   89.07%   90.33%   +1.26%     
==========================================
  Files          26       31       +5     
  Lines        3093     3290     +197     
==========================================
+ Hits         2755     2972     +217     
+ Misses        338      318      -20     
Impacted Files Coverage Δ
sepal_ui/aoi/aoi_model.py 87.34% <57.14%> (+0.05%) ⬆️
sepal_ui/mapping/value_inspector.py 89.18% <89.18%> (ø)
sepal_ui/mapping/sepal_map.py 79.60% <90.26%> (ø)
sepal_ui/scripts/utils.py 97.02% <94.73%> (-0.21%) ⬇️
sepal_ui/mapping/basemaps.py 96.77% <96.77%> (ø)
sepal_ui/frontend/styles.py 100.00% <100.00%> (ø)
sepal_ui/mapping/draw_control.py 100.00% <100.00%> (ø)
sepal_ui/mapping/fullscreen_control.py 100.00% <100.00%> (ø)
sepal_ui/mapping/layer.py 100.00% <100.00%> (ø)
sepal_ui/mapping/map_btn.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f49625...6ed17a2. Read the comment docs.

@12rambau 12rambau requested a review from dfguerrerom May 23, 2022 17:32
Copy link
Collaborator

@dfguerrerom dfguerrerom left a comment

Choose a reason for hiding this comment

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

I've made some comments in SepalMap. I'll wait until the "max_zoom" error is fixed so I can test the map on live, and I will keep testing the new feats.

sepal_ui/mapping/mapping.py Outdated Show resolved Hide resolved
sepal_ui/mapping/basemaps.py Show resolved Hide resolved
sepal_ui/mapping/mapping.py Outdated Show resolved Hide resolved
sepal_ui/mapping/mapping.py Show resolved Hide resolved
sepal_ui/mapping/v_inspector.py Outdated Show resolved Hide resolved
sepal_ui/mapping/mapping.py Outdated Show resolved Hide resolved
sepal_ui/mapping/v_inspector.py Outdated Show resolved Hide resolved
sepal_ui/mapping/mapping.py Show resolved Hide resolved
sepal_ui/mapping/map_btn.py Show resolved Hide resolved
sepal_ui/mapping/v_inspector.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@12rambau 12rambau marked this pull request as ready for review May 30, 2022 10:55
Copy link
Collaborator

@dfguerrerom dfguerrerom left a comment

Choose a reason for hiding this comment

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

I just made a single comment but I'm approving the review. It looks really nice, great job prr!

sepal_ui/mapping/sepal_map.py Show resolved Hide resolved
@12rambau 12rambau merged commit 4bec294 into master May 31, 2022
@12rambau 12rambau deleted the sepalmap branch May 31, 2022 15:08
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.

there's a dot in the bottomright corner of the map set the basemaps as member of the SepalMap
2 participants