-
Notifications
You must be signed in to change notification settings - Fork 91
Add soft link and external links in JLD2 #686
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #686 +/- ##
==========================================
+ Coverage 87.40% 87.53% +0.13%
==========================================
Files 37 38 +1
Lines 4588 4676 +88
==========================================
+ Hits 4010 4093 +83
- Misses 578 583 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This PR is quite big, and I think it is important to review this code very carefully since it is AI-generated. Is it possible to get the AI to split the PR up into smaller pieces? For example, can soft-links and external-links be added separately? Caching is, in general, really difficult to get right. Can this be removed from the basic feature PRs and added afterwards as a performance optimization? |
|
Hi @nhz2 , Yeah, no worries. I like the fact that i got a working implementation without that much effort on my side. It allows us to add regression tests and then improve the code from there. I agree that the caching logic is probably BS and should be removed. |
|
Yes, it's also very cool as a proof of concept to know this and chunks can be added without making breaking changes. |
54b806d to
fe80145
Compare
fe80145 to
756e831
Compare
3daca85 to
1bdc060
Compare
e68b376 to
4a551e2
Compare
|
@nhz2 I've done another major overhaul of this PR. This improved performance and brought down the number of additional code lines introduced here. Splitting the PR into separate bits does not really seem useful to me at this point. Joining these in a single struct makes sense, because there will never be more than these three types and it allows for type stability. |
Add Soft Links and External Links Support
This PR implements soft links and external links in JLD2, bringing enhanced HDF5 compatibility and enabling cross-file
dataset references.
Changes
Core Link System
within the same file), and external links (references to datasets in other files)
API
Linkis not exported because it is a very generic name and not many people will be using this.Internal Changes
Documentation
Usage Example