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

Add collection_property helper to simplify building property filters #331

Closed
wants to merge 4 commits into from

Conversation

soxofaan
Copy link
Member

@soxofaan soxofaan commented Sep 30, 2022

While working on #328, I thought of this proof of concept to simplify specifying simple collection property filters.

Usage example:

from openeo import collection_property

cube = con100.load_collection(
    "S2",
    properties=[
        collection_property("eo:cloud_cover") <= 75,
        collection_property("platform") == "Sentinel-2B",
    ]
)

note that properties is now a list (instead of a dictionary),
containing these collection_property operator tricks that are translated to the proper process graphs

@soxofaan
Copy link
Member Author

could also be considered a solution for #246

@m-mohr
Copy link
Member

m-mohr commented Sep 30, 2022

I like that, although could "collection_property" be a bit shorter? :-) prop, field, property, key or so.

Because it is longer than before now:

"eo:cloud_cover": lambda v: v <= 75 # old
collection_property("eo:cloud_cover") <= 75 #proposal
prop("eo:cloud_cover") <= 75 # better?

@soxofaan
Copy link
Member Author

soxofaan commented Oct 4, 2022

It's indeed longer, but only in number of characters.
It's simpler in terms of symbols or concepts: just a function call and a comparison, while with the existing approach you have to understand how lambda works (which isn't the case for most beginners)

collection_property is indeed a bit long, but making it shorted like prop, field, property or key is risky because the user is expected (or will be tempted) to import it globally (from openeo import thing) and you don't want thing to collide with existing globals. property is for example a python builtin global, field and key are very popular variable names with high chance on collision. prop is too short or cryptic in my opinion.

@soxofaan
Copy link
Member Author

soxofaan commented Oct 4, 2022

I also expect that users are going to copy-paste from example snippets (or use AI driven code generators in the future 😛 ), so I don't think that the extra typing effort for collection_property is that much of an issue

@m-mohr
Copy link
Member

m-mohr commented Oct 4, 2022

But if they copy/use AI anyway, then you can simply stick with "eo:cloud_cover": lambda v: v <= 75 :-P

Another reason not to use collection_filter: We also have other processes that use the property filtering, which are not related to collections, e.g. filter_labels. So I'd argue property or filter - or if you wanna namespace it openeo_filter - are better choices... :-)

@soxofaan
Copy link
Member Author

soxofaan commented Oct 5, 2022

property and filter are a no-go as they are builtin functions, you don't want a user to accidentally overwrite these. Namespacing like openeo_filter looks a bit weird compared to the rest of the python API.
value_filter could be an option, because that's what it is: a helper to build a {"from_parameter": "value"} filter process graph.

However, I personally still like the concreteness of something like

load_collection(...
    properties=[
        collection_property("eo:cloud_cover") <= 75,

Another alternative is using the terminology "metadata property" as used in the description of load_collection:

load_collection(...
    properties=[
        metadata_property("eo:cloud_cover") <= 75,

@m-mohr
Copy link
Member

m-mohr commented Oct 5, 2022

I just checked it against and my claim that filter_label also uses this is wrong. In all other "logical expressions" (filter_labels, count, aray_filter) you have direct values to work against and don't need the special handling.

In the schema the subtype is called metadata-filter by the way.

@soxofaan soxofaan force-pushed the collection-property-filtering branch from 59d725c to f143c7a Compare November 14, 2023 11:05
@soxofaan
Copy link
Member Author

We decided to push this forward, still sticking with collection_property so that users can start using this. We'll flag it as an experimental feature to keep the option open for a better name if one pops up

…lding property filters for load_collection
@soxofaan soxofaan closed this Nov 14, 2023
@soxofaan soxofaan deleted the collection-property-filtering branch November 14, 2023 12:52
@soxofaan
Copy link
Member Author

(squashed and) merged in 40be725

soxofaan added a commit that referenced this pull request Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants