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

ee.FeatureCollection to ee.Dictionary #373

Merged
merged 3 commits into from
Nov 9, 2024
Merged

ee.FeatureCollection to ee.Dictionary #373

merged 3 commits into from
Nov 9, 2024

Conversation

fitoprincipe
Copy link
Member

@fitoprincipe fitoprincipe added the kind: enhancement New feature or request label Nov 5, 2024
@fitoprincipe fitoprincipe requested a review from 12rambau November 5, 2024 01:11
Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.90%. Comparing base (a45bdb1) to head (5bc2ad0).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #373      +/-   ##
==========================================
+ Coverage   90.85%   90.90%   +0.04%     
==========================================
  Files          27       27              
  Lines        1663     1671       +8     
  Branches       78       78              
==========================================
+ Hits         1511     1519       +8     
  Misses        130      130              
  Partials       22       22              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

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

As a general appreciation I love this PR as it solves an issue I open n the Google issue tracker a long time ago.

Appart from the comments I made on specific code, I'm asking myself a question, what error do we want to raise when the user specify a keycolumn that leads to duplicate id ?

  1. not the same number of parameters ? then we go all the way to the Dictionary setup and and values and key will not have the same length
  2. cannot use duplicate keys ? then same but we never use distinct on the keys

I think 2 is preferable (and that's what you coded) but I don't know if it was on purpose.

@fitoprincipe
Copy link
Member Author

Answering to your general comment. Yes, it was on purpose. I actually wrote it down in the docstings. This is the perfect moment to express my follow up idea: I tried to applied a grouped reducer (ee.Reducer.group) in ee.FeatureCollection.reduceColumns but it didn't work. Although it's not complex to do it, I think a shortcut would be helpful, and could be an alternative way of doing what this PR is trying to implement. I will create a new PR with my idea.

@fitoprincipe fitoprincipe merged commit dbf22ac into main Nov 9, 2024
12 checks passed
@fitoprincipe fitoprincipe deleted the fc-to-dict branch November 9, 2024 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants