Add serialization logic for complex numbers#50885
Closed
anjali411 wants to merge 10 commits intogh/anjali411/89/basefrom
Closed
Add serialization logic for complex numbers#50885anjali411 wants to merge 10 commits intogh/anjali411/89/basefrom
anjali411 wants to merge 10 commits intogh/anjali411/89/basefrom
Conversation
[ghstack-poisoned]
This was referenced Jan 21, 2021
Closed
Contributor
💊 CI failures summary and remediationsAs of commit 3b5f8da (more details on the Dr. CI page):
🕵️ 5 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
SplitInfinity
left a comment
There was a problem hiding this comment.
Nice! We should add a test for build_complexdoublelist or remove the deserialization logic for it. Other than that, I think this is ready to land.
Comment on lines
+32
to
+35
| buffer = io.BytesIO() | ||
| torch.jit.save(scripted, buffer) | ||
| buffer.seek(0) | ||
| loaded = torch.jit.load(buffer) |
There was a problem hiding this comment.
I think you can use self.getExportImportCopy here, which will save and load from a buffer for you.
Comment on lines
+523
to
+524
| } else if (class_name == "build_complexdoublelist") { | ||
| elem_type = ComplexDoubleType::get(); |
Contributor
Author
There was a problem hiding this comment.
good catch! this change shouldn't be here because complex lists are not supported yet
[ghstack-poisoned]
[ghstack-poisoned]
Contributor
Author
|
@SplitInfinity updated the PR based on the comments! |
SplitInfinity
approved these changes
Jan 27, 2021
| def test_pickle(self): | ||
| class ComplexModule(torch.nn.Module): | ||
| def __init__(self): | ||
| super(ComplexModule, self).__init__() |
There was a problem hiding this comment.
Suggested change
| super(ComplexModule, self).__init__() | |
| super().__init__() |
Differential Revision: [D26094906](https://our.internmc.facebook.com/intern/diff/D26094906) [ghstack-poisoned]
Contributor
|
@anjali411 merged this pull request in 2de4ecd. |
Collaborator
|
Unlanding. This appears to have broken several builds. Example failure: |
Contributor
|
This pull request has been reverted by dfdb154. |
gchanan
reviewed
Jan 28, 2021
| case StorageType::Kind: | ||
| case NumberType::Kind: | ||
| case FloatType::Kind: | ||
| case ComplexDoubleType::Kind: |
Contributor
There was a problem hiding this comment.
I guess this already existed but this naming looks inconsistent -- aren't these referring to python types, so this should be ComplexType?
There was a problem hiding this comment.
I was under the impression that there will eventually be a ComplexFloatType as well but if not, then I agree that it should be ComplexType.
Contributor
Author
There was a problem hiding this comment.
hmm I didn't know that we follow the python naming here. yeah should be ComplexType!
gchanan
reviewed
Jan 28, 2021
| // Unpickle a tensor | ||
| bool quantized = class_name == "_rebuild_qtensor"; | ||
| rebuildTensor(quantized); | ||
| } else if (module_name == "builtins" && class_name == "complex") { |
Contributor
There was a problem hiding this comment.
above I see some stuff about floatlists -- we don't need to handle complexlists because we don't define them in e.g. native_functions yet?
Contributor
There was a problem hiding this comment.
it's also a little unclear to me if this is handling endianness -- do you know?
There was a problem hiding this comment.
The real and imaginary parts of the complex number are pickled using pushIValue which seems to take care of it endianness. See the relevant excerpts of the pickling/unpickling code below.
void Pickler::pushDouble(double value) {
push<PickleOpCode>(PickleOpCode::BINFLOAT);
// Python pickle format is big endian, swap.
push<double>(swapDouble(value));
}
double Unpickler::readFloat() {
AT_ASSERT(sizeof(double) == 8);
double big_endian = read<double>();
double little_endian;
// Pickle floats are big endian, so reverse the bytes
auto big_endian_ptr = reinterpret_cast<const char*>(&big_endian);
std::reverse_copy(
big_endian_ptr,
big_endian_ptr + sizeof(big_endian),
reinterpret_cast<char*>(&little_endian));
return little_endian;
}
laurentdupin
pushed a commit
to laurentdupin/pytorch
that referenced
this pull request
Apr 24, 2026
Summary: Pull Request resolved: pytorch#50885 Test Plan: Imported from OSS Reviewed By: SplitInfinity Differential Revision: D26094906 Pulled By: anjali411 fbshipit-source-id: 7b2614f3ee4a30c4b4cf04aaa3432988b38a0721
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
Differential Revision: D26094906