-
Notifications
You must be signed in to change notification settings - Fork 4
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 support for new filtering ops #581
Conversation
33d210d
to
6a802bb
Compare
dd47208
to
79928c3
Compare
require_bounding_box=False, | ||
require_polygon=False, | ||
require_raster=False, | ||
labels=schemas.LogicalFunction( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is much longer and harder to read than the old version. is it necessary to write it this way? I'm sure the answer is "yes", and I know it doesn't matter much since this will all live in the backend, but...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldnt think of a more intuitive name. I could enumerate it into And
, Or
and Not
like the client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just meant that we're going from one class Filter
with a few easy arguments to 5(?) classes with more complicated arguments and doubling the lines of code in the process. I don't have any real suggestions here as I'm not deep enough in the code to understand if we need to be this specific with all of these different classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5(?) classes
Which ones are you referring to?
Filter
s are constructed using 2 classes.
Condition
(e.g. ==, !=, >, etc)LogicalFunction
s (e.g. AND, OR, NOT).
@@ -313,7 +312,7 @@ def test_generate_segmentation_data( | |||
|
|||
for image in dataset.get_datums(): | |||
uid = image.uid | |||
sample_gt = dataset.get_groundtruth(uid) | |||
sample_gt = dataset.get_groundtruth(uid) # type: ignore - issue #604 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's causing there to be a type issue on this line with these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed type ignore. There is red highlighting but it passes pre-commit.
Some of these do fail pre-commit. This is an issue documented in #604.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was to be the last filtering PR, so I was expecting issues like this one to be fixed. I'm ok with pushing this into another PR if you think that's best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the last one to work on filter structure. These type ignores are related to pyright issues that only affect json generation in the client.
Feature
This PR implements a new filtering schema for Valor and connects the refactored query generator #597 to the API.
This includes support for logical operations (AND, OR, NOT) and the ability to apply filters per table.
API Endpoints
Filters no longer use query parameters in api calls.
Examples