Skip to content

Conversation

@tylerriccio33
Copy link
Contributor

Summary

This is a PR to create aggregation methods for the Validate class. Users can compare summary statistics like sum, sd, avg, etc. I'm trying to make this as modular as possible and dynamically generate the methods themselves.

This is a WIP so no need to merge. I'll have to at least implement the most common stats.

Related GitHub Issues and PRs

This PR references the conversation about implementing higher order methods and wrappers specifically for summary stats.

@tylerriccio33
Copy link
Contributor Author

Alright so I implemented a bare bonescol_sum_* and col_avg_*. Do you have any immediate concerns or pointers? If not I'll go ahead and implement more aggregators.

Also, why is this test failing "FAILED tests/test_validate.py::test_validation_with_all_validation_types_pandas - assert (18 / 21) >= 0.9" ? It failed in the other local-tests branch you merged in where there were no real changes, so makes me think it's spurious. Any ideas?

@tylerriccio33 tylerriccio33 marked this pull request as ready for review December 1, 2025 19:58
@rich-iannone
Copy link
Member

rich-iannone commented Dec 1, 2025

The .col_sum_*() and .col_avg_*() methods look great! No concerns at all from looking at the new files (I'll come up with the set of icons soon, as before).

And I think it's a recent release of Narwhals that broke the Pandas test. I'll fix that and once that's merged, you can merge this PR to main and it should no longer be a problem.

@tylerriccio33 tylerriccio33 marked this pull request as draft December 1, 2025 20:19
@MarcoGorelli
Copy link
Contributor

MarcoGorelli commented Dec 2, 2025

And I think it's a recent release of Narwhals that broke the Pandas test. I'll fix that and once that's merged, you can merge this PR to main and it should no longer be a problem.

sorry about that, though i'm a little confused as to how, given that we're meant to be running pointblack tests in our ci https://github.com/narwhals-dev/narwhals/actions/runs/19853532931/job/56885727968 -taking a look 👀


this passes for me locally 🤷 any idea what the issue might be?

@rich-iannone
Copy link
Member

@MarcoGorelli looking back into the CI logs, I actually can't find any trace of the failure in the merged PR's commits. And it never happened for me in local test runs.

I only suspected it might have something to do with Narwhals because of the recent update around that time (but I should have remembered to check the Narwhals CI for the downstream libraries testing). That said, the failure on CI was fixed with only a small test adjustment.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants