Skip to content

Conversation

@majosm
Copy link
Collaborator

@majosm majosm commented Jul 1, 2025

Duplication checks have been off by default due to some subpackages still having code that duplicates. Now that inducer/arraycontext#221 has been merged, it may be possible to enable the checks by default. Likely there are still a few spots in arraycontext/meshmode/grudge (and the production versions of those) that need fixing, but hopefully they should be minor.

Draft because:

@majosm majosm force-pushed the enable-duplication-checks-by-default branch 2 times, most recently from 2dcf277 to 76ef440 Compare July 1, 2025 18:26
@majosm majosm force-pushed the enable-duplication-checks-by-default branch from 76ef440 to 485ed49 Compare July 1, 2025 18:33
@majosm majosm force-pushed the enable-duplication-checks-by-default branch 2 times, most recently from a7a4501 to 1802263 Compare July 7, 2025 16:15
@inducer
Copy link
Owner

inducer commented Jul 7, 2025

#616 should clean up some of the bpr nonsense.

@majosm
Copy link
Collaborator Author

majosm commented Jul 8, 2025

Note to self: These should probably be err_on_<whatever>=__debug__ rather than err_on_<whatever>=True.

@majosm majosm force-pushed the enable-duplication-checks-by-default branch from ee7709e to 30d3321 Compare July 9, 2025 14:02
@majosm majosm force-pushed the enable-duplication-checks-by-default branch from 30d3321 to 5bf59af Compare July 18, 2025 18:46
@majosm
Copy link
Collaborator Author

majosm commented Aug 18, 2025

@inducer I looked into what specifically was making the mapper-created duplicate check in Deduplicator slow. Seems it's because the EqualityComparer cache isn't preserved across the different rec calls in Deduplicator. I attempted a fix here. Confirmed that it works in illinois-ceesd/drivers_y3-prediction#91. Thoughts?

I also took a stab at caching equality for mapper cache dict lookups here, but at least with my implementation it didn't show any benefit (number of equality rec calls went down a few percent, but overall time actually got a little worse).

@majosm majosm force-pushed the enable-duplication-checks-by-default branch from 5bf59af to b2845b3 Compare August 28, 2025 15:57
@majosm majosm force-pushed the enable-duplication-checks-by-default branch from b2845b3 to 12f37d3 Compare October 1, 2025 15:37
@majosm majosm marked this pull request as ready for review October 1, 2025 19:55
@majosm majosm requested a review from inducer October 1, 2025 19:56
Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

LGTM subject to:

  • Let's defer sorted removal until we're on constantdict

@majosm majosm force-pushed the enable-duplication-checks-by-default branch 2 times, most recently from 41fa277 to 8545054 Compare October 7, 2025 14:44
@majosm majosm force-pushed the enable-duplication-checks-by-default branch from 8545054 to 624657b Compare October 7, 2025 14:52
@majosm majosm force-pushed the enable-duplication-checks-by-default branch from 624657b to dcbfdb2 Compare October 7, 2025 15:23
@majosm majosm merged commit 5084f28 into inducer:main Oct 7, 2025
10 checks passed
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