Skip to content

Conversation

@FBruzzesi
Copy link
Member

@FBruzzesi FBruzzesi commented Jul 16, 2025

What type of PR is this? (check all applicable)

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below

Overall, this is not a real change. What this PR achieves:

  1. It flips the logic on the positive point of view: from "result is not " to "result is "
  2. It uses boolean operators instead of if-statements/conditionals to "reduce" complexity

@FBruzzesi FBruzzesi changed the title refactor: Simplify combine_metadata refactor: Simplify combine_metadata Jul 16, 2025
Comment on lines +552 to +564
result_expansion_kind = (
result_expansion_kind & expansion_kind
if i > 0
else expansion_kind
)

if metadata.has_windows:
result_has_windows = True
result_has_windows |= metadata.has_windows
result_n_orderable_ops += metadata.n_orderable_ops
if metadata.preserves_length:
result_preserves_length = True
if not metadata.is_elementwise:
result_is_not_elementwise = True
if not metadata.is_scalar_like:
result_is_not_scalar_like = True
if not metadata.is_literal:
result_is_not_literal = True
if metadata.is_filtration:
n_filtrations += 1
result_preserves_length |= metadata.preserves_length
result_is_elementwise &= metadata.is_elementwise
result_is_scalar_like &= metadata.is_scalar_like
result_is_literal &= metadata.is_literal
n_filtrations += int(metadata.is_filtration)
Copy link
Member

@dangotbanned dangotbanned Jul 17, 2025

Choose a reason for hiding this comment

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

I'm pretty impressed by this 🥳

I hadn't seen a real world use-case for __iand__ before, but this looks like it!

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks @FBruzzesi !

i remembered some previous discussion where someone had brought up that this was less efficient, but i'm not bothered enough about the difference, and this is clearer anyway, so let's ship it 🚢

@dangotbanned dangotbanned mentioned this pull request Jul 17, 2025
8 tasks
@MarcoGorelli MarcoGorelli merged commit 5b8cd7b into main Jul 17, 2025
31 of 32 checks passed
@MarcoGorelli MarcoGorelli deleted the refactor/simplify-combine_metadata branch July 17, 2025 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants