-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: update RNTuple files to the v1 spec #164
Conversation
When you're ready to have these included, and for me to make a release of scikit-hep-testdata with them in it, press the button that requests me as a reviewer. I don't see it when the "Ready for review" button gets pressed, but I do see it when I'm assigned as a reviewer. (I'm glad this is going forward!) |
There's some files that were generated from ATLAS workflows, so I can't reproduce them. I'll write new scripts to make new ones with the features we were testing. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
=======================================
Coverage 72.38% 72.38%
=======================================
Files 3 3
Lines 134 134
=======================================
Hits 97 97
Misses 37 37 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
This is now on par with the test files we previously had (in fact, there are a couple of new things), so it's ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I see that this is adding a lot of files and they all have v1
in their names. That's good: they won't conflict in any user's individual caches (in ~/.local
) for the old files. But since RNTuple is adopting a 4-digit version number, 1.0.0.0
, maybe all of those zeros should be included in the file names, in case we eventually need 1.0.0.1
(for only a few, I hope). To be kind to any filesystems that interpret .
in a special way, it could be 1-0-0-0
.
You decide on that, and when you're ready, merge this PR. If I don't notice the merge, remind me on Slack and I'll make a release.
Thanks! That sounds good. It makes sense to have the full version even though they probably won't be replaced in the future. I'll make that change. |
I made the name changes mentioned above, so I'll merge it now. |
The v1 spec of RNTuple is supposed to be released this week, so I'll be updating all the old files, and the generating scripts if necessary. I'll give them new names to avoid caching issues.