Save and Load objects#778
Conversation
…ethods: to_binary(std::ostream) and from_binary(std::istream) Clearer separation: Save and Load handle file IO, while to_binary and from_binary only write to ostream or istream implementation classes provide to_binary_dispatch(std::ostream) and from_binary_dispatch(std::istream)
…ectly because the char* is not terminated by \0. Potentially illegal memory access
manuschneider
force-pushed
the
save_load
branch
from
f1d349c to
f45c7c8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #778 +/- ##
==========================================
+ Coverage 36.23% 36.25% +0.02%
==========================================
Files 214 214
Lines 33132 33140 +8
Branches 13171 13159 -12
==========================================
+ Hits 12005 12016 +11
- Misses 19184 19191 +7
+ Partials 1943 1933 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c00d2384da
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
The C++ coding style we use uses PascalCase for function names. We should be consistent with it for the new functions especially the functions in the changing files have already follow the coding style. |
I checked the Cytnx API. We use one-word names, both upper- and lowercase ( But I suggest deviating from Google's style guide here, because I could not find any instance where we currently use PascalCase for functions or methods, and we should not introduce yet another convention for new functions. Another reason to deviate from Google style guide is that we want to have a consistent API on Python and C++. In Python, snake_case is recommended for function names, and therefore I would also suggest this for our API (and because we already use it a lot). I renamed some functions in this branch because they started with an underscore (which should be avoided) and did not have very meaningful names (e.g., |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64b25e4bc0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19ac5b06f8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }, | ||
| py::arg("fname"), py::arg("dtype"), py::arg("count") = cytnx::cytnx_int64(-1)) | ||
| [](const std::string &fname, const unsigned int &dtype, const cytnx::cytnx_int64 &count, | ||
| const bool restore_device) { return cytnx::Tensor::Load(fname, restore_device); }, |
There was a problem hiding this comment.
The Python Tensor.Fromfile binding now takes dtype/count plus a fourth flag, but the lambda still calls Tensor::Load instead of Tensor::Fromfile. That means raw binary inputs are parsed as serialized .cytn tensors, and the dtype/count arguments are silently ignored. In this patch this also became more misleading because the new fourth parameter is bool restore_device, so callers passing an integer device ID will be coerced to true/false and still not control placement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13ad2a01c2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } else { | ||
| this->_impl->_syms.clear(); |
There was a problem hiding this comment.
Fresh evidence after the earlier “fixed” thread: Bond::from_binary now clears only _syms in the Nsym_in == 0 path, but it still leaves _qnums/_degs untouched for version-0 payloads. With the new in-place Load_ path, loading a non-symmetric or old-format bond into an already-initialized symmetric/new-format bond can retain stale degeneracy metadata and make later logic (e.g. format-dependent getDegeneracy behavior) inconsistent with the file contents.
Useful? React with 👍 / 👎.
| if (len_name != 0) { | ||
| char *cname = (char *)malloc(sizeof(char) * len_name); | ||
| f.read(cname, sizeof(char) * len_name); | ||
| this->_impl->_name = std::string(cname); | ||
| this->_impl->_name = std::string(cname, len_name); |
There was a problem hiding this comment.
UniTensor::from_binary only assigns _name when len_name != 0; otherwise it leaves the previous name in place. This becomes a regression with the new in-place Load_ API: loading a file whose tensor name is empty into an existing named tensor preserves the old name, so the loaded object's metadata no longer matches what was serialized.
Useful? React with 👍 / 👎.
2 participants
addresses #157 and #660
The Pickle support can already be checked and merged at this stage, and HDF5 can be implemented subsequently (check #660 for this).