Skip to content

Complex number support#249

Merged
agronholm merged 4 commits intoagronholm:masterfrom
chillenb:complex
Aug 13, 2025
Merged

Complex number support#249
agronholm merged 4 commits intoagronholm:masterfrom
chillenb:complex

Conversation

@chillenb
Copy link
Copy Markdown
Contributor

@chillenb chillenb commented Nov 17, 2024

Changes

Fixes #248

Adds support for complex numbers under tag 43000.

Checklist

If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):

  • You've added tests (in tests/) added which would fail without your patch
  • You've updated the documentation (in docs/, in case of behavior changes or new
    features)
  • You've added a new changelog entry (in docs/versionhistory.rst).

If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.

Updating the changelog

If there are no entries after the last release, use **UNRELEASED** as the version.
If, say, your patch fixes issue #999, the entry should look like this:

- Fixed big bad boo-boo in the encoder (#999 <https://github.com/agronholm/cbor2/issues/999>_; PR by @yourgithubaccount)

If there's no issue linked, just link to your pull request instead by updating the
changelog after you've created the PR.

@chillenb chillenb force-pushed the complex branch 4 times, most recently from 3ff08b3 to 1c4da62 Compare November 17, 2024 22:54
@coveralls
Copy link
Copy Markdown

coveralls commented Nov 17, 2024

Coverage Status

coverage: 94.444% (-0.3%) from 94.757%
when pulling be4142e on chillenb:complex
into 0b8d689 on agronholm:master.

@chillenb
Copy link
Copy Markdown
Contributor Author

Not sure what's happening with the fuzzer. It doesn't look like it ran properly?

@agronholm
Copy link
Copy Markdown
Owner

Not sure what's happening with the fuzzer. It doesn't look like it ran properly?

Don't worry about that. It never worked right, and I haven't been able to reach the person who added it. I haven't decided what to do about it yet.

@chillenb
Copy link
Copy Markdown
Contributor Author

just rebased, it still works!

@agronholm
Copy link
Copy Markdown
Owner

I really need to get this done. I have too many projects to look after :(

@agronholm
Copy link
Copy Markdown
Owner

I had a look and it looks pretty straightforward. I'll just double check the test cases and I should be able to then merge this.

@agronholm
Copy link
Copy Markdown
Owner

One more thing: where did you get the spec for this?

@agronholm
Copy link
Copy Markdown
Owner

One more thing: where did you get the spec for this?

Ah...I guess it's from the "short form" here: https://www.iana.org/assignments/cbor-tags/template/43000

@agronholm
Copy link
Copy Markdown
Owner

Do you intend to also add support for the companion tag of 43001 (complex number arrays)?

@chillenb
Copy link
Copy Markdown
Contributor Author

Do you intend to also add support for the companion tag of 43001 (complex number arrays)?

Probably not. I am not convinced 43001 is the right way to handle complex arrays. There are lots of tags for multidimensional arrays of various types, and 43001 seems too limited to interoperate well with those. What do you think?

@agronholm
Copy link
Copy Markdown
Owner

Do you intend to also add support for the companion tag of 43001 (complex number arrays)?

Probably not. I am not convinced 43001 is the right way to handle complex arrays. There are lots of tags for multidimensional arrays of various types, and 43001 seems too limited to interoperate well with those. What do you think?

I guess you're right. Let's leave it out for now then.

@agronholm
Copy link
Copy Markdown
Owner

Decode-only support might still on the table for 43001. But I think we'll cross that bridge when we come to it.

@agronholm
Copy link
Copy Markdown
Owner

If you could just address the two review comments, I could get this merged.

@chillenb
Copy link
Copy Markdown
Contributor Author

I've thought it over some more and am not really opposed to implementing 43001, even if it's not the best way to do complex arrays. At least it's a way.

@agronholm agronholm merged commit 88eabb7 into agronholm:master Aug 13, 2025
14 of 15 checks passed
@agronholm
Copy link
Copy Markdown
Owner

Thanks!

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.

Complex numbers!

3 participants