-
Notifications
You must be signed in to change notification settings - Fork 11
Species of Interest TaxaList Filter #777
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
Conversation
✅ Deploy Preview for antenna-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| logger.debug(f"Filtering by taxalist {taxalist_id} taxa_ids {taxa}") | ||
|
|
||
| # filter by the exact determination | ||
| query_filter = Q(determination__in=taxa) |
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.
Can you use a sub query instead? If you generate the list of taxa in python first and then pass it back, it may be too long. Some regional taxa lists will have over a few thousand items.
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.
Example:
Lines 2343 to 2346 in 1adb99d
| Classification.objects.filter(detection__occurrence=self) | |
| .filter( | |
| score__in=models.Subquery( | |
| Classification.objects.filter(detection__occurrence=self) |
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.
Actually I don't think a subquery is possible with our parents_json field. We can come back to that later. Also the first filter can occur on the Taxa view. Instead of the Occurrences view. Ideally both, but Taxa is the most important for the first demo.
config/api_router.py
Outdated
| router.register(r"detections", views.DetectionViewSet) | ||
| router.register(r"occurrences", views.OccurrenceViewSet) | ||
| router.register(r"taxa", views.TaxonViewSet) | ||
| router.register(r"taxalists", views.TaxaListViewSet) |
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.
Will you try mounting under /taxa? /taxa/lists? Like the ml prefix. We will eventually move all taxonomy related things into a taxa app. If it causes a headache then no worries.
ami/main/api/serializers.py
Outdated
|
|
||
|
|
||
| class TaxaListSerializer(serializers.ModelSerializer): | ||
| taxa = serializers.PrimaryKeyRelatedField(queryset=Taxon.objects.all(), many=True) |
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 don't think the TaxaListSerializer needs to return the actual taxa in the list. Try adding a link to the filtered taxa view instead like this example in SourceImageCollectionSerializer which links to a filtered SourceImages view:
antenna/ami/main/api/serializers.py
Lines 1099 to 1108 in c18ff50
| def get_source_images(self, obj) -> str: | |
| """ | |
| Return URL to the captures endpoint filtered by this collection. | |
| """ | |
| return reverse_with_params( | |
| "sourceimage-list", | |
| request=self.context.get("request"), | |
| params={"collections": obj.pk}, | |
| ) |
Perhaps in the detail view, but I don't think the taxa list is necessary in either right now and it will be long!
ami/main/api/views.py
Outdated
| query_param = "taxalist" | ||
|
|
||
| def filter_queryset(self, request, queryset, view): | ||
| taxalist_id = request.query_params.get(self.query_param) |
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'm glad you are using the IntegerField here for validating the query param! Here is how we are doing it in other filters. You don't need to check for the taxalist_id first if you set required to False.
Lines 864 to 868 in c18ff50
| def filter_queryset(self, request, queryset, view): | |
| collection_id = IntegerField(required=False).clean(request.query_params.get(self.query_param)) | |
| if collection_id: | |
| # Here the queryset is the Occurrence queryset | |
| return queryset.filter(detections__source_image__collections=collection_id) |
| """ | ||
| Return URL to the taxa endpoint filtered by this taxalist. | ||
| """ | ||
| return reverse_with_params( |
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.
Thanks for finding this!
| return super().list(request, *args, **kwargs) | ||
|
|
||
|
|
||
| class TaxonTaxaListFilter(filters.BaseFilterBackend): |
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.
OccurrenceTaxaListFilter and TaxonTaxaListFilter are almost identical. If we can't reuse the filter, will you at least update the docstring in the filter classes to make it clear they are doing the same type of recursive search? Something like "Query for all taxa in the requested TaxaList, and any of their children that are in the the TaxaList, recursively"
Thank you!
|
@annavik will you review the frontend changes on this one? We will likely have a Taxonomy section soon in the new Project sub-menus as well! |
|
@annavik I pushed one simple method of hiding the Taxa List filter if there are no lists configured for the current project. I couldn't see how to add a If we do more of these, we could add a Anyway, let us know what you think of the other changes. We can improve the lists feature over time. |
annavik
left a comment
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.
So glad you could include some FE updates @mohamedelabbas1996 ! 👏
Just let me know if you want to have a chat about anything. The multi fetch problem I want to think about a bit more, I think no show stopper though!
|
Thanks for the fixes @mohamedelabbas1996, this looks good to me now! :) I pushed an update to make it possible to pass custom data to filter controls. Thanks to that we can now skip the multi fetching. See commit 7cd7738 if you are curious about the solution. I will approve this now! |
Thanks for pointing that out, Anna! I was actually following the pattern discussed in issue #660, where we're aiming to use object_id to refer to the actual ID and object for the object itself (e.g., project_id vs. project). Not sure if we’re still aligning with that convention, happy to adjust if needed! |
I see, I understand it's a bit confusing since we have been a bit inconsistent here! I do like having Let's keep this filter key as is, if the plan is to update other filter keys in a near future? :) @mihow any thoughts? |
|
@annavik I'm glad you noticed this detail! And I am glad you like the
I'm sorry we haven't updated them all at once! |
Thanks for sharing the fix. It seems like that approach opens the door for a lot of flexibility. |



Summary
This PR introduces a Taxa List filter for occurrences and taxa views, enabling filtering based on taxons in a Taxa List . The filter ensures that Occurrences and Taxa can be filtered by their presence in a Taxa List or if their ancestors belong to a specified Taxa List. Additionally, Taxa Lists can be managed from the admin dashboard, allowing users to Create new Taxa Lists, Add taxa to a Taxa List and Link Taxa Lists to projects.
List of Changes
OccurrenceTaxaListFilter: Enables filtering Occurrence objects where determination is in a TaxaList or has an ancestor in the list.TaxonTaxaListFilter: Enables filteringTaxonobjects where the taxon is in aTaxaListor has an ancestor in the list.Users can now filter occurrences and taxa by selecting a Taxa List from a dropdown.
Related Issues
#745
How to Test the Changes
Only Occurrences with a determination in the selected TaxaList or its ancestors are displayed.
Changing the TaxaList filter updates the results accordingly.
Screenshots
Deployment Notes
No database migrations required.
Checklist