typing(aggregation): Add structure and none based weightings#655
Draft
ValerianRey wants to merge 3 commits intomainfrom
Draft
typing(aggregation): Add structure and none based weightings#655ValerianRey wants to merge 3 commits intomainfrom
ValerianRey wants to merge 3 commits intomainfrom
Conversation
- The idea is to make it explicit that some weightings are based on the structure of the matrix (sum, mean, random) and that some are independent of their input (constant), by separating the part where we extract the structure (or the none for constant) and the part where we use this structure (or the none for constant) to make a vector of weigths.
ValerianRey
added
package: aggregation
cc: typing
github-actions
bot
changed the title
github-actions
bot
changed the title
github-actions
bot
changed the title
ValerianRey
commented
Apr 17, 2026
| """Computes the vector of weights from the input stat.""" | ||
|
|
||
| def __call__(self, stat: Tensor, /) -> Tensor: | ||
| def __call__(self, stat: object, /) -> Tensor: |
Contributor
Author
There was a problem hiding this comment.
This is wrong. This type should be _T. Before, Tensor was the public equivalent of both Matrix and PSDMatrix, so it worked, but it's quite wrong.
If we do type this as _T, we need Matrix and PSDMatrix to become public, and I think we need users to cast to Matrix and PSDMatrix in order to use call.
Finding a workaround would be huge (this problem + the issue of having a gap between public and private type, solved at once).
| dtype: torch.dtype | ||
|
|
||
|
|
||
| def extract_structure(matrix: Matrix) -> Structure: |
Contributor
There was a problem hiding this comment.
This could be not only for Matrices but any non-scalar?
Contributor
Author
There was a problem hiding this comment.
Yes. That's my plan for a future PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft for now (I think I broke the documentation, even if it still builds) but I'd like to know your opinion about this @PierreQuinton. Seems a bit more rigorous than what we had before.
This fixes the issue that currently, weightings are bound to operate on tensors (which doesn't really make sense, as they're supposed to operate on some stats which may not be represented as a single tensor).
This also removes the responsibility from ConstantWeighting of checking if the matrix' shape[0] matches the number of weights.