Use estimated count on full dataset view #5446
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
fiftyone/core/collections.py
Outdated
| return True | ||
| if isinstance(self, fov.DatasetView): | ||
| if not ( | ||
| self._pipeline() |
There was a problem hiding this comment.
Note that _pipeline() can be expensive for some views. EG if a view contains SortBySimilarity it can even run model inference!!!
That's why I wrote the existing if statement in this function in a seemingly funny way: I first check things that are super cheap and only call _pipeline() last once I've determined that the view is simple and thus _pipeline() will be fast. Taking advantage of short-circuiting.
I'd request that a similar treatment be applied to any additional logic added to this method.
Side note, I've been staring at this first if for a minute and can't figure out what it is saying(!)
There was a problem hiding this comment.
ohh thanks for calling this out! I was wondering why we used _pipeline() and stages here
regarding the if, its just factoring out the not, so fully expanded, it translates to not pipeline and not clips and not groups
There was a problem hiding this comment.
Yeah I don't like the refactor either, it's much harder to understand imo.
There was a problem hiding this comment.
Updated the syntax...
Side note, kinda funny how the bot summary described this syntax as simplified and streamlined 🤖
There was a problem hiding this comment.
the rabbit sees the glass half full 😃
|
Why would a view have a negative start? |
|
I guess it doesn't matter, len(ds.view()) should be fast as well |
Slices from the end to see the tail |
Oh well if it's at the tail, then it's not the full collection? |
|
We overwrite the slice operator so that dataset[-N:] triggers a skip aggregation and to get the value of skip, there's a hidden len(dataset.view()) that is previously excluded in this condition |
|
Ah ok so really the optimization is for |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fiftyone/core/collections.py (1)
10625-10640: LGTM! The implementation correctly determines full collections.The logic properly handles both regular and grouped datasets by checking:
- For regular datasets - no stages, not clips, and not GROUP media type
- For grouped datasets - single SelectGroupSlices stage with no pipeline
Consider performance implications of _pipeline() call.
The method calls
_pipeline()which could be expensive for some views. Consider moving this check earlier in the grouped dataset condition to take advantage of short-circuiting.if ( self._dataset.media_type == fom.GROUP and len(self._stages) == 1 and isinstance(self._stages[0], fos.SelectGroupSlices) - and not self._pipeline() ): + if not self._pipeline(): + return True - return True
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/core/collections.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e / test-e2e
- GitHub Check: build
| if isinstance(self, fod.Dataset) and self.media_type != fom.GROUP: | ||
| return True |
There was a problem hiding this comment.
| if isinstance(self, fod.Dataset) and self.media_type != fom.GROUP: | |
| return True | |
| # Case 1: full non-group dataset | |
| if isinstance(self, fod.Dataset) and self.media_type != fom.GROUP: | |
| return True |
| if isinstance(self, fov.DatasetView): | ||
| # pylint:disable=no-member | ||
| if ( | ||
| not len(self._stages) | ||
| and not self._is_clips | ||
| and self._dataset.media_type != fom.GROUP | ||
| ): | ||
| return True | ||
|
|
||
| if ( | ||
| self._dataset.media_type == fom.GROUP | ||
| and len(self._stages) == 1 | ||
| and isinstance(self._stages[0], fos.SelectGroupSlices) | ||
| and not self._pipeline() | ||
| ): | ||
| return True |
There was a problem hiding this comment.
lets add comments about the cases
| if isinstance(self, fov.DatasetView): | |
| # pylint:disable=no-member | |
| if ( | |
| not len(self._stages) | |
| and not self._is_clips | |
| and self._dataset.media_type != fom.GROUP | |
| ): | |
| return True | |
| if ( | |
| self._dataset.media_type == fom.GROUP | |
| and len(self._stages) == 1 | |
| and isinstance(self._stages[0], fos.SelectGroupSlices) | |
| and not self._pipeline() | |
| ): | |
| return True | |
| if isinstance(self, fov.DatasetView): | |
| # Case 2: Empty non-group dataset view | |
| # pylint:disable=no-member | |
| if ( | |
| not len(self._stages) | |
| and not self._is_clips | |
| and self._dataset.media_type != fom.GROUP | |
| ): | |
| return True | |
| # Group dataset with all slices selected and no additional stages | |
| if ( | |
| self._dataset.media_type == fom.GROUP | |
| and len(self._stages) == 1 | |
| and isinstance(self._stages[0], fos.SelectGroupSlices) | |
| and not self._pipeline() | |
| ): | |
| return True |
| # pylint:disable=no-member | ||
| if ( | ||
| not len(self._stages) | ||
| and not self._is_clips |
There was a problem hiding this comment.
Is clips the only case? Or is it any generated dataset?
What changes are proposed in this pull request?
How is this patch tested? If it is not, please explain why.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Summary by CodeRabbit