Skip to content

refactor(aggregation): Make gramian weighted aggregator generic#645

Closed
ValerianRey wants to merge 5 commits intomainfrom
make-gramian-weighted-aggregator-generic
Closed

refactor(aggregation): Make gramian weighted aggregator generic#645
ValerianRey wants to merge 5 commits intomainfrom
make-gramian-weighted-aggregator-generic

Conversation

@ValerianRey
Copy link
Copy Markdown
Contributor

@ValerianRey ValerianRey commented Apr 15, 2026

Draft PR where I'm trying to add setters to aggregators.

It seems required to have proper typing of the gramian_weighting field of the aggregator, and for that we would need the GramianWeightedAggregator to be generic on the type of gramian_weighting.

This also requires defining the weighting before the aggregator (so reordering the code).

Before continuing further, what do you think about this @PierreQuinton?

We could maybe make the structural change (making GramianWeightedAggregator generic) and let other contributors add the setters.

BTW i'm not sure if the typevar should be covariant or contravariant. By default it's invariant.

@ValerianRey ValerianRey added package: aggregation cc: refactor Conventional commit type for any refactoring, not user-facing, and not typing or perf improvements labels Apr 15, 2026
@github-actions github-actions bot changed the title make gramian weighted aggregator generic refactor(aggregation): Make gramian weighted aggregator generic Apr 15, 2026
Copy link
Copy Markdown
Contributor

@PierreQuinton PierreQuinton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good. Regarding variance, I think it would be covariant as we want CAGrad to be a subclass of GramianWeightedAggregator[CAGradWeighting] which is a subclass of GramianWeightedAggregator[Weighting[PSDMatrix]].

@ValerianRey
Copy link
Copy Markdown
Contributor Author

I think this is good. Regarding variance, I think it would be covariant as we want CAGrad to be a subclass of GramianWeightedAggregator[CAGradWeighting] which is a subclass of GramianWeightedAggregator[Weighting[PSDMatrix]].

Are you sure? Got a type error unless I set contravariance to True. No idea what I'm doing though.

@ValerianRey
Copy link
Copy Markdown
Contributor Author

I think you're right. Idk why the type checker thinks that aggregator.weighting is just an object when aggregator is a WeightedAggregator (i also made them generic)

@ValerianRey
Copy link
Copy Markdown
Contributor Author

I guess there could be a bug with ty because Weighting is contravariant.

@ValerianRey
Copy link
Copy Markdown
Contributor Author

Instead of doing that let's just add type hint to the weighting and gramian_weighting, specialized for each aggreg.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cc: refactor Conventional commit type for any refactoring, not user-facing, and not typing or perf improvements package: aggregation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants