Skip to content
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

Added ability to sort Complex numbers. #107

Closed
wants to merge 2 commits into from

Conversation

annoybot
Copy link

Why?

I have a lot of unit tests to write which need to check arrays of complex numbers.

Being able to sort an array of Complex, makes it easier to write these tests (I can just sort the actual results and the expected results and compare).

Patrick Nadeau added 2 commits June 20, 2022 15:13
Added a test for it.

FIXME: The srot test does not actually check the results.
@bluss
Copy link
Contributor

bluss commented Jun 21, 2022

This change means that complex can now be used with operators such as <, <=, which needs some rationale, is there a good reason?

@annoybot
Copy link
Author

Indeed the complex numbers are not an ordered field, so as you point out the < operation makes no mathematical sense for it.

I actually solved my problem in a different way by creating a temporary struct:

    #[derive(PartialEq, Eq, Copy, Clone, Hash, Debug, Default, PartialOrd, Ord)]
    struct CplxSortAdaptor<T:Float> {
        pub re: T,
        pub im: T,
    }

I was then able to write a function sort_lex<F:Float>(list: &mut[Complex<F>]) which copies the list to be sorted into a list of CplxSortAdaptor, sorts it, then copies the results back into list.

Not super efficient, but OK, since it's only needed in my unit tests.

I don't know why I didn't see this solution sooner.

I propose to close this PR for the reasons you cited. Thanks for your feedback.

@cuviper
Copy link
Member

cuviper commented Jun 21, 2022

Closing, but we can follow up on how to use custom sorting in #106.

@cuviper cuviper closed this Jun 21, 2022
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.

None yet

3 participants