feat(rust): right-align numeric columns

[alicja-januszkiewicz]

closes #7378

[alexander-beedie]

Hey, thanks for this :)

I put a follow-up comment on the original issue; do you think you could update the PR to include an option for fixing the number of floating point decimal places offered? I suspect right-alignment would be a lot more attractive/useful then.

Also, if you could make this opt-in for now, via a suitable Config option and associated environment variable, we can test it out a bit before deciding to change the default behaviour 👍

[alicja-januszkiewicz]

Hey, thanks for the reply. This looks doable for me, I will give it a shot.

[alexander-beedie]

Hey, thanks for the reply. This looks doable for me, I will give it a shot.

Fantastic :)
I'd suggest introducing two new config options via the following environment vars:

• POLARS_FMT_DECIMAL_PLACES: default to None, otherwise a +ve integer.
• POLARS_FMT_TABLE_CELL_NUMERIC_ALIGNMENT: default to None, allowing "LEFT", "RIGHT", "CENTER" - same asPOLARS_FMT_TABLE_CELL_ALIGNMENT, which it should override for numeric cols (if not None).

Add them into the Config class on the python side (can pattern after set_tbl_cell_alignment), and then we can experiment with the two settings independently to see what might work best for a potential new default 👍

[alexander-beedie]

Nice, looks like it's almost there - and POLARS_FMT_FLOAT_PRECISION is a good choice (fits well with the existing float_precision param in write_excel/write_csv) 👍

[alicja-januszkiewicz]

ready for review @alexander-beedie :)

[alexander-beedie]

Apologies for the delay; I managed to convince myself that GitHub was acting up last night after accidentally disabling JavaScript in my browser...

Have checked out the branch and compiled it up, so...

• Right-alignment of numeric cols:
  • Looks good to me - let's consider that part done.
• Float precision:
  • Instead of the env var, let's use the same mechanism as set_float_format & get_float_format to set/load the integer value of decimal places directly; reading the env var for each float is expensive given that this function is used by Display (I didn't connect those dots earlier).
  • Experimenting with more values, it seems that the switch to scientific notation throws things off earlier than desirable, as the .len() > 9 check kicks in much too soon (for instance, with 6dp); we should set this quite a bit higher when in fixed-dp mode. Maybe even as high as 19?? I'm open to counter-offers on competing values for that, heh. We need to maintain fixed-dp into at least the hundreds of millions (or early/mid billions?) though 💭

With those two changes (and a rebase) we should be good to go :)

[alexander-beedie]

FYI, I added a mechanism for allowing directly applied values (those not mediated by env vars) to better participate in Config load/save/scoping yesterday: #7696.

[alicja-januszkiewicz]

Right I've updated the PR to use that mechanism for float_precision.

This is sort of ready for review again, but there is a bit of an issue regarding the float_precision option. Namely, that when using set_float_format & get_float_format instead of using env vars, there isn't a straightforward way of checking if the option was ever set in the first place, so I have reserved the value of 255 (u8:MAX) to indicate an unset value. I admit this is a bit lazy but the alternative would require something like FLOAT_PRECISION: RwLock<Option<AtomicU8>>, which seems a bit over-complicated. Or perhaps that would have been better? What do you think?

[alexander-beedie]

I admit this is a bit lazy but the alternative would require something like FLOAT_PRECISION: RwLock<Option<AtomicU8>>, which seems a bit over-complicated. Or perhaps that would have been better? What do you think?

I think that's fine; keeps it simple - can you raise an error if anyone tries to set a nonsense value? Probably anything above 16, which (if I recall correctly) is about the precision limit of an f64. Certainly anything much above that is living in real denial about floating point accuracy :)

Have had family in town recently, so got a bit backlogged; will download the latest updates at the w/e and run through them (could you do a quick rebase?) 👍

[alicja-januszkiewicz]

Sorry I was a bit busy myself over the Easter. I've tried to rebase tonight but I can't figure out how to without adding in hundreds of commits to the PR. I will try to sort this out next weekend.

[alexander-beedie]

Sorry I was a bit busy myself over the Easter. I've tried to rebase tonight but I can't figure out how to without adding in hundreds of commits to the PR. I will try to sort this out next weekend.

No problem; I think it'll be a great addition :) We have a 4 day weekend coming up here (national holidays), so I'll have plenty of time to review and/or help out.

I can't figure out how to without adding in hundreds of commits

A rebase + force-push should do the trick? Initially it'll look like you're incorporating tons of things, but after the force you should just be left with your own changes.

[alicja-januszkiewicz]

@alexander-beedie Hey sorry I only just got around to fixing this, I've added the check for float precision being greater than 16 and rebased.

A rebase + force-push should do the trick?

Yeah that's what I tried originally, but I ended up getting some commits from main in the commits/changes tabs to this PR. It was probably because I merged main into this branch at some earlier point, so I tried removing that merge commit first and that seems to have fixed it.

[stevenlis]

I'd suggest introducing two new config options via the following environment vars:

• POLARS_FMT_DECIMAL_PLACES: default to None, otherwise a +ve integer.
• POLARS_FMT_TABLE_CELL_NUMERIC_ALIGNMENT: default to None, allowing "LEFT", "RIGHT", "CENTER" - same asPOLARS_FMT_TABLE_CELL_ALIGNMENT, which it should override for numeric cols (if not None).

Add them into the Config class on the python side (can pattern after set_tbl_cell_alignment), and then we can experiment with the two settings independently to see what might work best for a potential new default 👍

Sorry to bother you guys. But is this still happening? @alexander-beedie @alicja-januszkiewicz

[alexander-beedie]

Sorry to bother you guys. But is this still happening? @alexander-beedie @alicja-januszkiewicz

Argh... this is 100% my fault; I must have missed some GitHub notifications and let it slip through the cracks. Short version is that "yes", I'd love to get this merged - it's a great contribution, and (potentially) could even become the default format.

@alicja-januszkiewicz; since this is my bad, I have checked out this branch and rebased/merged locally; unfortunately I don't seem able to push it back out, though the "Maintainers are allowed to edit this pull request" option has been checked. I'll see if I can work out why not and will update accordingly 🤔

error: failed to push some refs to 'https://github.com/alicja-januszkiewicz/polars.git'
To https://github.com/alicja-januszkiewicz/polars.git
!	refs/heads/right_aligned_numbers:refs/heads/right_aligned_numbers	[remote rejected] 

[ritchie46]

@alexander-beedie do you use the github cli? This always works much, much easier for pushing to a PR.

[alexander-beedie]

@alexander-beedie do you use the github cli?

I do not, but I'm willing to try ;)

---

Update: Ah! It's ok, all working now.
Just needed to update my GitHub personal access token with the workflow capability.

[alicja-januszkiewicz]

Hi,

I'd be happy to continue working on this, but I'm not sure what is the next step here - this is still pending review right? @alexander-beedie

[alexander-beedie]

I'd be happy to continue working on this, but I'm not sure what is the next step here - this is still pending review right? @alexander-beedie

From my perspective I think it's in good shape; @ritchie46 or @orlp, would one of you mind having a quick look? (Note that this is a new formatting option, not enabled by default). Then let's merge and we can iterate further if necessary :)

[orlp]

Instead of usingu8::MAX to signal 'no option set' I think we should use an Option<T> initialized with None.

[alexander-beedie]

@alicja-januszkiewicz: want to take this one? :) I'm going to take a look at the max precision value again, as we should consider Decimal too (support for which is going to be turned on by default soon).

[alicja-januszkiewicz]

Sure, I'll try to get this over by the end of the week.

[alicja-januszkiewicz]

I've changed the type of FLOAT_PRECISION from AtomicU8 to RwLock<Option<usize>>.

I've picked usize for now as that's the type the write! and format! macros expect, so this saves having to repeatedly cast the value.

I've wrapped it in an RwLock to hopefully retain whatever protection AtomicU8 was providing, as there doesn't seem to be an atomic equivalent of Option.

[svaningelgem]

@stinodego : This is a PR that I think could be merged as @alicja-januszkiewicz is still active on it (congratz mate!) and I think the feature would be well received.
CC: @alexander-beedie

[alexander-beedie]

@stinodego : This is a PR that I think could be merged as @alicja-januszkiewicz is still active on it (congratz mate!) and I think the feature would be well received.

CC: @alexander-beedie

Definitely. Give me a few minutes with this one; has been a (too) long road getting this one over the line (which is on me 😅)

[alexander-beedie]

Ok, rebased it and added an example to the docstring; pending a look from @orlp, I think we're ready to ship! 🥳
@alicja-januszkiewicz: sincere thanks for this one - and for your patience :)