Repository navigation
Use proper precision when format not specified - #11145
Conversation
|
Thanks! Would need a test and a change log. @mhvk or anyone who reviews, feel free to adjust milestone if I set it wrongly. |
|
I reverted it to |
| } | ||
|
|
||
| # Set the precision, if set, otherwise use numpy default | ||
| pops = np.get_printoptions() |
There was a problem hiding this comment.
Yes, it makes sense that this doesn't work with arrays. I fear we'll need to explicitly set one of the numpy formatting arguments (maybe just precision itself? but a different default?), since those will be used if one just expands {self.value}. I remember the machinery changed a little while ago, so probably good to start by looking what it presently allows.
There was a problem hiding this comment.
Should be fixed - while being backwards compatible - by using np.array2string in the conditional
if format not in formats:
raise ValueError(f"Unknown format '{format}'")
elif format is None:
if precision is None:
# Use default formatting settings
return f'{self.value}{self._unitstr:s}'
else:
# np.array2string properly formats arrays as well as scalars
return f'{np.array2string(self.value, precision=precision, floatmode="fixed")}{self._unitstr:s}'
The CI/Python3.8 job is failing for an unknown reason - looks like a connection error. Locally, it works perfectly fine. In any case, I will mark this PR as ready for review
|
Failure does look transient. I restarted the CI. |
|
Does this PR need a changelog entry? |
This PR is marked as a bug fix, so yes, please add a change log entry. Thanks! |
995bd17 to
1bacfaa
Compare
|
Done! (sorry that it took a while...) |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Looks good, thanks so much! Two comments left, both nitpicky...
| return f'{self.value}{self._unitstr:s}' | ||
| else: | ||
| # np.array2string properly formats arrays as well as scalars | ||
| return f'{np.array2string(self.value, precision=precision, floatmode="fixed")}{self._unitstr:s}' |
There was a problem hiding this comment.
Total nitpick, but to me this reads clearer as
return np.array2string(self.value, precision=precision, floatmode="fixed") + self._unitstr
| astropy.units | ||
| ^^^^^^^^^^^^^ | ||
|
|
||
| - Use proper precision when printing a ``Quantity`` in case precision format not |
There was a problem hiding this comment.
Maybe "when creating a string from a Quantity"? It affects to_string() and __str__.
There was a problem hiding this comment.
Should be fixed (and migrated to milestone 4.0.5) in acb7cc3
|
p.s. Need to move the changelog entry to 4.0.5 to match the milestone... |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Thanks, looks good now, but I did manage to find one more nitpick. Sorry! When you change that, could you also squash the commits? (If not sure how, see https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#how-to-squash -- am also happy to do it for you).
| - Ensure ``keepdims`` works for taking ``mean``, ``std``, and ``var`` of | ||
| ``Quantity``. [#11198] | ||
|
|
||
| - Use proper precision when creating a string from a ``Quantity`` in case precision format is not |
There was a problem hiding this comment.
Sorry to have another nitpick, but I think this suggestion of mine doesn't actually convey to a reader what changed. Maybe
- For ``Quantity.to_string()``, ensure that the precision argument is also
used when the format is not latex. [#11145]
There was a problem hiding this comment.
Fixed in 179bf32 , also squashed this bug fix into one commit.
acb7cc3 to
179bf32
Compare
Use proper precision when format not specified
…format Use proper precision when format not specified
Manual backport of #11145 (Use proper precision when format not specified)
Description
The precision of a
Quantitywas not properly printed in case the precision was specified andformatwas empty. This should be fixed by definingformat_speca bit earlier, and using this when parsing the "unadorned" string.Fixes #11131