Skip to content

Fix from_others issue with SQLgraph.#182

Merged
JoOkuma merged 20 commits intoroyerlab:mainfrom
yfukai:fix_sqlgraph_save_issue
Nov 10, 2025
Merged

Fix from_others issue with SQLgraph.#182
JoOkuma merged 20 commits intoroyerlab:mainfrom
yfukai:fix_sqlgraph_save_issue

Conversation

@yfukai
Copy link
Copy Markdown
Contributor

@yfukai yfukai commented Oct 24, 2025

Graphs generated by RegionPropsNodes could not be converted from RustWorkXGraph to SQLGraph. This PR intends to solve this issue. This is a quick fix, and later it may make sense to refactor the overall logic.

I also updated curry to partial since I believe the later is sufficient here.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 24, 2025

Codecov Report

❌ Patch coverage is 81.25000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.17%. Comparing base (cb4770a) to head (5939c92).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/tracksdata/nodes/_regionprops.py 20.00% 2 Missing and 2 partials ⚠️
src/tracksdata/utils/_dtypes.py 85.71% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #182      +/-   ##
==========================================
- Coverage   88.22%   88.17%   -0.05%     
==========================================
  Files          51       51              
  Lines        3593     3612      +19     
  Branches      617      625       +8     
==========================================
+ Hits         3170     3185      +15     
- Misses        254      256       +2     
- Partials      169      171       +2     

☔ 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 24, 2025

@yfukai, I might have introduced this bug in #170.

I'll carefully review the pickling behavior from your PR

Ideally, and I think it's still the case, when an array has a fixed length, it should return a polars series of columns of array or list. I will add some tests for this.

@JoOkuma
Copy link
Copy Markdown
Member

JoOkuma commented Nov 3, 2025

Hey @yfukai, I was able to simplify your changes due to #170, but I broke the tests when merging with main.
My changes are at yfukai#7. Can you see if you agree with it?


for col in node_attrs.columns:
if col != DEFAULT_ATTR_KEYS.T:
graph.add_node_attr_key(col, node_attrs[col].first())
Copy link
Copy Markdown
Contributor Author

@yfukai yfukai Nov 4, 2025

Choose a reason for hiding this comment

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

Maybe this should be inherited from the other graph? Or we can handle this with a new PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@yfukai, unfortunately, we don't store the default value at the graph level. This is something I want to add when reworking the add_node_attr_key to have something better.
Do you see another way of getting it from the other graph?

Copy link
Copy Markdown
Contributor Author

@yfukai yfukai left a comment

Choose a reason for hiding this comment

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

I merged yfukai#7 and started to work on that! I left a comment about the default value.

@JoOkuma
Copy link
Copy Markdown
Member

JoOkuma commented Nov 5, 2025

@yfukai, thanks. I left a reply to your comment. If you can inherit the existing information, that would be great. I'm going to rework the add_{node|edge}_attr_key soon, so this will soon change.

Let's remove the centroid prop from the failing test.

I'm trying to fix the benchmark at #190 , without much success.

@yfukai
Copy link
Copy Markdown
Contributor Author

yfukai commented Nov 7, 2025

Hi @JoOkuma, Thanks!
Can you check if yfukai#8 makes sense? I think current impl fails with non-array and non-None pickable objects.

This is something I want to add when reworking the add_node_attr_key to have something better.

This sounds great! I don't have an idea other than saving a dict of attr_key -> default_value mapping in graphs...

@yfukai
Copy link
Copy Markdown
Contributor Author

yfukai commented Nov 7, 2025

We can work on regionprops default value a bit more (since some props are array-typed) but I created another issue (#191) for that.

@yfukai
Copy link
Copy Markdown
Contributor Author

yfukai commented Nov 7, 2025

Hi @JoOkuma thank you for your comment! For me this PR is fine to be merged!

@JoOkuma
Copy link
Copy Markdown
Member

JoOkuma commented Nov 10, 2025

Hi @yfukai, I'm merging this PR. The benchmark error is unrelated; I'm trying to fix it on #190

@JoOkuma JoOkuma merged commit 75f28d6 into royerlab:main Nov 10, 2025
6 of 7 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.

3 participants