Skip to content

Saving dtypes as metadata for roundtrip consistency#262

Merged
JoOkuma merged 16 commits into
royerlab:mainfrom
yfukai:from_other_roundtrip
Apr 13, 2026
Merged

Saving dtypes as metadata for roundtrip consistency#262
JoOkuma merged 16 commits into
royerlab:mainfrom
yfukai:from_other_roundtrip

Conversation

@yfukai

@yfukai yfukai commented Feb 17, 2026

Copy link
Copy Markdown
Contributor

This is a follow-up on the dtype, assuming #260 has been merged.
In this PR, the graphs store the dtypes of the fields as metadata, enabling robust recovery of the original dtypes during round-trip from_other conversions.

@yfukai yfukai marked this pull request as draft February 17, 2026 06:42
@yfukai

yfukai commented Feb 17, 2026

Copy link
Copy Markdown
Contributor Author

Notes:

  • I should also store the default values. Serializing the polars dataframes is useful to store the values?
  • The roles of AttrSchema overlap with the current metadata. This could be SQLGraph-specific machinery.

@yfukai

yfukai commented Feb 18, 2026

Copy link
Copy Markdown
Contributor Author

Both tasks are done. I'll review the code for SQLgraph and mark this open for review.

@codecov-commenter

codecov-commenter commented Feb 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.33%. Comparing base (4ed06c1) to head (521a683).

Files with missing lines Patch % Lines
src/tracksdata/graph/_sql_graph.py 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #262      +/-   ##
==========================================
- Coverage   87.34%   87.33%   -0.02%     
==========================================
  Files          56       56              
  Lines        4703     4705       +2     
  Branches      829      832       +3     
==========================================
+ Hits         4108     4109       +1     
  Misses        379      379              
- Partials      216      217       +1     

☔ 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

JoOkuma commented Feb 27, 2026

Copy link
Copy Markdown
Member

@yfukai sorry for the delay.
I'll review this once #260 is merged. Everything looks good so far.

@JoOkuma JoOkuma left a comment

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.

Great PR @yfukai,

I appreciate all the tests you're adding.
I left a few minor comments.

Comment thread src/tracksdata/graph/_sql_graph.py Outdated
Comment thread src/tracksdata/graph/_sql_graph.py
yfukai and others added 3 commits March 1, 2026 16:38
@JoOkuma

JoOkuma commented Mar 2, 2026

Copy link
Copy Markdown
Member

@yfukai, I'm a bit puzzled. GitHub diff shows little difference.
Do you think I did something wrong when merging the conflicts?
We can revert the changes.

@yfukai

yfukai commented Mar 3, 2026

Copy link
Copy Markdown
Contributor Author

Maybe the PR was merged with #259 and now in the repo... I can still fix the code that you commented and continue with this PR! Or instead, we can revert 259 and merge this first.

@JoOkuma

JoOkuma commented Mar 3, 2026

Copy link
Copy Markdown
Member

Gotcha, you can address the comments here.
It's already merged, so we can leave it as it is.
Thanks @yfukai

@yfukai

yfukai commented Apr 8, 2026

Copy link
Copy Markdown
Contributor Author

Hi @JoOkuma, I'm sorry for the late response! Finally, I could take time on this.
#262 (comment)
About this, I believe this verbose pattern was necessary since we serialize the metadata at the whole metadata level in the SQLGraph. Does this make sense for you?

@JoOkuma

JoOkuma commented Apr 10, 2026

Copy link
Copy Markdown
Member

That makes sense @yfukai, and I'm glad you're back.
I think this PR is ready to merge.
Is that OK for you?

@yfukai

yfukai commented Apr 13, 2026

Copy link
Copy Markdown
Contributor Author

Hi I'm sorry for the absence! Yes, feel free to merge this!

@yfukai

yfukai commented Apr 13, 2026

Copy link
Copy Markdown
Contributor Author

I'll continue working on #268

@JoOkuma JoOkuma merged commit 6ea4fbf into royerlab:main Apr 13, 2026
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