Skip to content

Adding is_in operator for AttrComparison#178

Merged
JoOkuma merged 12 commits intoroyerlab:mainfrom
yfukai:filter_revisited
Oct 21, 2025
Merged

Adding is_in operator for AttrComparison#178
JoOkuma merged 12 commits intoroyerlab:mainfrom
yfukai:filter_revisited

Conversation

@yfukai
Copy link
Copy Markdown
Contributor

@yfukai yfukai commented Oct 20, 2025

I found it useful to have is_in for AttrComparison to filter a set of tracks. I'm not sure if I'm going to a right direction, so inputs are welcome!

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 20, 2025

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.03%. Comparing base (a188502) to head (7032a01).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/tracksdata/attrs.py 93.10% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
+ Coverage   87.95%   88.03%   +0.08%     
==========================================
  Files          50       50              
  Lines        3411     3444      +33     
  Branches      585      596      +11     
==========================================
+ Hits         3000     3032      +32     
- Misses        247      248       +1     
  Partials      164      164              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JoOkuma
Copy link
Copy Markdown
Member

JoOkuma commented Oct 20, 2025

Hi @yfukai, awesome PR, I was not familiar with TypeGuard.
I did my revision in this PR, yfukai#5, could you check if you agree my changes?

I removed the handling of SQLAlchemy because today everything is processed when they are polars dataframe, maybe in the future that could be done through the ORM to avoid this additional overhead of SQL -> polars.

@yfukai
Copy link
Copy Markdown
Contributor Author

yfukai commented Oct 20, 2025

Thank you for your review @JoOkuma! I merged the PR.

@yfukai
Copy link
Copy Markdown
Contributor Author

yfukai commented Oct 20, 2025

Sorry I should have been a bit more careful. It looks like the filter is building an SQL at least for the SQLGraph. Let me have a closer look!

@yfukai
Copy link
Copy Markdown
Contributor Author

yfukai commented Oct 20, 2025

In the test, lhs is int for RustWorkXGraph and <sqlalchemy.orm.attributes.InstrumentedAttribute object at 0x1391602c0> for SQLGraph.

@yfukai
Copy link
Copy Markdown
Contributor Author

yfukai commented Oct 20, 2025

For SQLGraph, it actually build a query! attr_filter.op(getattr(table, str(attr_filter.column)), attr_filter.other)
For RustWorkXGraph, a function that maps a value to bool seems to be required as it is called here def _filter(attrs: dict[str, Any]) -> bool:`.

@JoOkuma
Copy link
Copy Markdown
Member

JoOkuma commented Oct 20, 2025

@yfukai, my bad, you're completely right and I introduced a bug into your code 😅
I forgot this was also used to filter the graph
I reverted the changes.
Let me know what you think, everything is ok to me

@yfukai
Copy link
Copy Markdown
Contributor Author

yfukai commented Oct 20, 2025

Hi thanks @JoOkuma! No worries, it's a great news in a sense!
I just dropped reference to pl.Expr in the in_op since it was introduced by my misunderstanding (RXGraphs are directly filtering the nodes and not using the polars here). If it makes sense for you please feel free to merge it!

@yfukai
Copy link
Copy Markdown
Contributor Author

yfukai commented Oct 21, 2025

Oh sorry that makes the test fail...

@yfukai
Copy link
Copy Markdown
Contributor Author

yfukai commented Oct 21, 2025

Sorry according to the test I found that AttrComparisons are also designed to work with polars dataframe! I've got the pl.Expr back to the in_op. Please feel free to merge it if you feel fine with that!

@JoOkuma JoOkuma merged commit ea2026c into royerlab:main Oct 21, 2025
7 checks passed
@JoOkuma
Copy link
Copy Markdown
Member

JoOkuma commented Oct 21, 2025

No problem, thanks for the great PR @yfukai

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