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

Add formatting options for Complex #170

Merged
merged 6 commits into from
Mar 25, 2017
Merged

Conversation

DonSheddow
Copy link
Contributor

This adds LowerExp and UpperExp traits for Complex, taking precision into account. Fixes #148

@cuviper
Copy link
Member

cuviper commented Feb 26, 2016

It failed CI because precision isn't stable in 1.0.0 -- not until 1.5.0!

If there's no other way to accomplish this, then we'll have to save this PR until we finally batch up breaking changes.

@homu
Copy link
Contributor

homu commented Feb 27, 2016

☔ The latest upstream changes (presumably #171) made this pull request unmergeable. Please resolve the merge conflicts.

@bluss
Copy link
Contributor

bluss commented Mar 1, 2016

Thanks for working on this!

What are the drawbacks of just forwarding the whole formatter? Basically call try!(self.re.fmt(f)); try!(write!(f, "+")); try!(self.im.fmt(f));. I guess it's unbeautiful, but it forwards every option into the inner scalars.

@cuviper
Copy link
Member

cuviper commented Mar 1, 2016

@bluss if there's any padding/alignment requested, your way would do it twice.
(But right now we're ignoring those things entirely...)

@DonSheddow
Copy link
Contributor Author

If we don't care about padding/alignment, then forwarding the formatter might be the best solution. It's succinct, relatively readable and we'd get scientific notation, precision etc. "for free". Another advantage of bluss's solution is that it's backwards compatible with 1.0.0 (I think).

If we want to support padding/alignment, however, we would probably have to do it manually. It might be best to make another macro to reduce repetition between Display, LowerExp and UpperExp, and put all the padding/precision code in there. That way we could reduce the body of the fmt functions to one line.

@cuviper
Copy link
Member

cuviper commented Mar 4, 2016 via email

@homu
Copy link
Contributor

homu commented Apr 13, 2016

☔ The latest upstream changes (presumably #164) made this pull request unmergeable. Please resolve the merge conflicts.

@hauleth hauleth modified the milestone: v0.2.0 Sep 18, 2016
@cuviper
Copy link
Member

cuviper commented Feb 10, 2017

@DonSheddow Care to revisit this? We just bumped to rust 1.8 in #263, so I think this won't require any breaking change to implement in full now, right?

@DonSheddow
Copy link
Contributor Author

I'm gonna start working on this now.

We want to support scientific notation and all formatting parameters -- fill/alignment, precision, etc., right? I'm not sure the '#' and '0' flags makes sense for complex numbers though. How about binary and hex? Python does not support displaying complex numbers as hex, so maybe we shouldn't either.

@DonSheddow
Copy link
Contributor Author

Come to think of it, representing floats as hex or binary doesn't make sense... I suppose the only traits we need to implement is Display, LowerExp and UpperExp.

Looking at https://doc.rust-lang.org/std/fmt/struct.Formatter.html, the only function that isn't stable and pre-1.8 is align. I'm not sure how we can do alignment without it. Any ideas?

@cuviper
Copy link
Member

cuviper commented Feb 11, 2017

Since this is generic code, I'd just do it for any T: LowerHex etc. It's not just for floats -- these could make sense for Gaussian integers, for instance.

Huh, align isn't even stable yet on nightly. Then I guess we just can't deal with that at all.

@DonSheddow
Copy link
Contributor Author

@cuviper How does this implementation look?

Since you can't specify a fill character without specifying alignment, I thought we'd just ignore both for now and implement fill when we can do alignment too. So this implementation just right-aligns and uses spaces as padding.

It's not quite clear what the '0' flag would do for complex numbers, but I thought it would make more sense if it applies the padding to each scalar individually, instead of at the beginning or end. So format("{:010}", Complex::new(1, 1)) returns 0001+0001i instead of 0000001+1i.

@cuviper
Copy link
Member

cuviper commented Mar 18, 2017

It's not quite clear what the '0' flag would do for complex numbers, but I thought it would make more sense if it applies the padding to each scalar individually, instead of at the beginning or end. So format("{:010}", Complex::new(1, 1)) returns 0001+0001i instead of 0000001+1i.

That's tricky. It looks fine to split the padding when real and imaginary are near equal length, but consider format("{:010}", Complex::new(1234567, 8)), we don't want 1234567+0001i. You already have the formatted real and imag at this point, so I think you could incorporate those lengths into the decision. Something like, pad the smaller toward the size of the larger; if they can be equal length with still more padding left, then spread the remaining padding equally.

If it looks too weird or difficult, we can decline to implement zero padding too. Is there any guidance to be had from how other languages pad complex numbers?

@DonSheddow
Copy link
Contributor Author

If it looks too weird or difficult, we can decline to implement zero padding too. Is there any guidance to be had from how other languages pad complex numbers?

I know atleast Python and Ruby ignores the zero pad flag when formatting complex numbers, but I don't know how any other languages do it.

That's tricky. It looks fine to split the padding when real and imaginary are near equal length, but consider format("{:010}", Complex::new(1234567, 8)), we don't want 1234567+0001i. You already have the formatted real and imag at this point, so I think you could incorporate those lengths into the decision. Something like, pad the smaller toward the size of the larger; if they can be equal length with still more padding left, then spread the remaining padding equally.

I see, format("{:010}", Complex::new(1234567, 8)) should just return 1234567+8i. Then format("{:012}", Complex::new(1234567, 8)) should return 1234567+008i, and format("{:018}", Complex::new(1234567, 8)) should probably return 001234567+0000008i, right? I don't think that would be too difficult to implement. I can give it a shot, but if the code ends up really ugly, it probably isn't a big deal to ignore the zero pad flag.

@cuviper
Copy link
Member

cuviper commented Mar 19, 2017

Yeah, you've understood my suggested padding, but I'm totally fine with ignoring this if other languages do.

@DonSheddow
Copy link
Contributor Author

Alright, I'm just gonna remove it for now, if it's not that important. We can always implement it later if we want.

@DonSheddow
Copy link
Contributor Author

I don't know why it doesn't pass CI by the way. Do I have to do anything else before this is merged?

@cuviper
Copy link
Member

cuviper commented Mar 21, 2017

Hmm, compiler panic on num-macros... it might finally be time to retire that one. I'll look into it.

@cuviper
Copy link
Member

cuviper commented Mar 25, 2017

Looks great, thanks!

@homu r+ 2784b89

@homu
Copy link
Contributor

homu commented Mar 25, 2017

⌛ Testing commit 2784b89 with merge 904d9b4...

homu added a commit that referenced this pull request Mar 25, 2017
Add formatting options for Complex

This adds LowerExp and UpperExp traits for Complex, taking precision into account. Fixes #148
@homu
Copy link
Contributor

homu commented Mar 25, 2017

☀️ Test successful - status

@homu homu merged commit 2784b89 into rust-num:master Mar 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants