Skip to content

MAINT: Tweak push_fields pass #748

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

Closed
wants to merge 2 commits into from
Closed

MAINT: Tweak push_fields pass #748

wants to merge 2 commits into from

Conversation

mtsokol
Copy link
Member

@mtsokol mtsokol commented May 7, 2025

Hi @willow-ahrens,

I might be wrong here but I think that reorder/aggregate rule in push_fields pass is never used as ¬issubset(A ∩ B, B) is always false regardless of inputs.

@mtsokol mtsokol requested a review from willow-ahrens May 7, 2025 12:38
@mtsokol mtsokol self-assigned this May 7, 2025
Copy link

codecov bot commented May 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
src/scheduler/optimize.jl 87.53% <ø> (-0.11%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@willow-ahrens
Copy link
Collaborator

notice that we use "issubsequence", not "issubset". I think the intention is to push a reorder through an aggregate when the fields of the reorder are in a different permutation than the fields of the argument, ignoring dropped or expanded dimensions. I'm not sure if we really should care to do this though, because in general we don't usually push reorders through aggregates. Let's wait for #750 to merge, and benchmark this change. I think the intention is to try to avoid permuting the output of an aggregate, if possible.

@mtsokol
Copy link
Member Author

mtsokol commented May 13, 2025

Thanks for an explanation - I'm still a bit confused by the if statement here. One thing would help me: Can you provide some exemplary values here for idxs_1 and idxs_2 so that the statement below evaluates to true?

!issubsequence(intersect(idxs_1, idxs_2), idxs_2)

Copy link
Collaborator

@willow-ahrens willow-ahrens left a comment

Choose a reason for hiding this comment

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

I'm happy to remove this rule if it passes tests and causes no regressions.

@mtsokol
Copy link
Member Author

mtsokol commented May 17, 2025

I'm happy to remove this rule if it passes tests and causes no regressions.

Let's first release what we have currently in the main branch (we'll be sure we have latest release without regression).
This weak can wait - I will test it separately, it isn't a priority.

@mtsokol
Copy link
Member Author

mtsokol commented May 21, 2025

Thanks for an explanation - I'm still a bit confused by the if statement here. One thing would help me: Can you provide some exemplary values here for idxs_1 and idxs_2 so that the statement below evaluates to true?

!issubsequence(intersect(idxs_1, idxs_2), idxs_2)

Explained in finch-tensor/finch-tensor-lite#31 (comment) it can evaluate to true (I missed the ordering part).

@mtsokol mtsokol closed this May 21, 2025
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.

2 participants