-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[Merged by Bors] - Add documentation to QueryCombinationIter
#5739
Conversation
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
Co-authored-by: James Liu <contact@jamessliu.com>
This will reflect the added context in this PR that is aware of what query items are and that they can be read-only or not.
Moved all links to the bottom of the doc comment. Links have been alphabetically ordered. Rebase notes: - After bevyengine#5205 got merged, the “Safety” section of `WorldQuery` has changed. I listed the mentioned methods for a better layout. I also removed the `WorldQuery::` prefix for readability. - I don't have enough knowledge of the previous and current state of `WorldQuery` types, so the documentation may be outdated somewhere. `WorldQuery` needs another review round.
Co-authored-by: Rob Parrett <robparrett@gmail.com>
Co-authored-by: Boxy <supbscripter@gmail.com>
/// In this context, a combination is an unordered subset of `K` elements. | ||
/// The number of combinations depend on how `K` relates to the number of entities matching the [`Query`] (called `N`): | ||
/// - if `K = N`, only one combination exists, | ||
/// - if `K < N`, there are <sub>N</sub>C<sub>K</sub> combinations (see the [performance section] of `Query`), |
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.
Do you have a screenshot of how this looks in the docs? Having <sub>
first looks wrong to me.
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 should render the same as here: NCK
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 not familiar with this notation. What does it mean?
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's a common notation of the binomial coefficient. You can also see the rendered docs by checking out the branch and doing the cargo doc thing.
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.
Interesting, if it's common enough I guess it's fine. Otherwise I think it would be good to switch to another notation or to describe it textually. I guess the problem is that we are constrained by the Markdown formatting so we can't use the "classical" notation.
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's still better than to write a fraction with factorials in it.
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.
👍
/// An iterator over `K`-sized combinations of query items without repetition. | ||
/// | ||
/// In this context, a combination is an unordered subset of `K` elements. | ||
/// The number of combinations depend on how `K` relates to the number of entities matching the [`Query`] (called `N`): | ||
/// - if `K = N`, only one combination exists, | ||
/// - if `K < N`, there are <sub>N</sub>C<sub>K</sub> combinations (see the [performance section] of `Query`), | ||
/// - if `K > N`, there are no combinations. |
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 very technically precise but might be a bit abstract or unapproachable for new users. Is there any value in changing the wording here to be more approachable, or is this not the place to do that?
I think I'm leaning towards the latter.
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.
idk, I just moved the documentation here from the methods, with minimal changes. Anyway, I think it's not too difficult to understand. You just need to have a basic understanding of combinatorics.
bors r+ |
# Objective - Document `QueryCombinationIter` ## Solution - Describe the item, add usage and examples - Copy notes about the number of query items generated from the corresponding query methods (they will be removed in #5742 ([motivation])) ## Additional notes - Derived from #4989 [motivation]: #4989 (comment)
Pull request successfully merged into main. Build succeeded: |
QueryCombinationIter
QueryCombinationIter
# Objective - Document `QueryCombinationIter` ## Solution - Describe the item, add usage and examples - Copy notes about the number of query items generated from the corresponding query methods (they will be removed in bevyengine#5742 ([motivation])) ## Additional notes - Derived from bevyengine#4989 [motivation]: bevyengine#4989 (comment)
# Objective - Document `QueryCombinationIter` ## Solution - Describe the item, add usage and examples - Copy notes about the number of query items generated from the corresponding query methods (they will be removed in bevyengine#5742 ([motivation])) ## Additional notes - Derived from bevyengine#4989 [motivation]: bevyengine#4989 (comment)
Objective
QueryCombinationIter
Solution
Query
methods documentation #5742 (motivation))Additional notes