Repository navigation
DOC/TST: console and unicode formatting support roundtrip - #19571
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
This is not quite right: the test that you added to only shows that a single unit name written out with console will roundtrip. But round-tripping is more than that: composite units should also roundtrip. This is not the case for all possibilities, though:
fluxunit = u.erg / (u.cm ** 2 * u.s)
u.Unit(fluxunit.to_string('console', fraction='inline')) == fluxunit
# True
u.Unit(fluxunit.to_string('console', fraction='multiline')) == fluxunit
# ValueError: ' erg
------
s cm^2' did not parse as unit: Invalid character at col 6 If this is meant to be a custom unit, define it with 'u.def_unit'. To have it recognized inside a file reader or other code, enable it with 'u.add_enabled_units'. For details, see https://docs.astropy.org/en/latest/units/combining_and_defining.html
If one were to make things round-trip, we'd need a parser that understands how fractions can be typeset in different ways. This is not horribly hard, but not clear it is very useful either.
Given the above, I think we could still do the small addition to the test function, though at some level superfluous (for a single unit, the output is identical to generic). But the docs should not be changed.
Anyway, my sense would be to close, or have a different PR that explain better why those units are for output only.
|
@mhvk Good point, I have forgotten to check |
|
@Julian-Harbeck - I think it would certainly be good to clarify that I think what could work is if in your test case, you also add something like and perhaps the same for unicode. Obviously, this would require adjusting the definition of If those tests indeed all pass for |
|
Great, then I will request another review from you when I implemented these changes. |
|
@Julian-Harbeck - just to check, will you have time to implement the fixes today? |
|
@astrofrog Yes will try so in the evening, did not had much time the last week. I would put the change log entry under |
|
I don't think this needs a change log. @mhvk , are you able to re-review since you requested changes? Thanks! 🙏 |
|
@mhvk I moved the documentation of For the test I opted to add an additional test for a composite unit to be tested with different
If you or @pllim would know what fails during the doc build I am happy to implement it. I guess it has something to do with the additional code line I added to demonstrade the roundtrip in |
|
Maybe forget about the comment on the docstring, there seems to be an issue related to IPython 9.13: |
f59ec6b to
744d36b
Compare
|
I did a rebase from the UI |
|
Docs are building now, but I just noted that the very next example fails to read back for >>> thz = u.THz.decompose().to_string('console', fraction=True)
>>> thz
'1*10^12 / s'
>>> u.Unit(thz)
Traceback (most recent call last):
…
ValueError: '1*10^12 / s' did not parse as unit: Syntax error parsing unit '1*10^12 / s'so this caveat should also be added to the docs. |
| [ | ||
| unit | ||
| for unit in u.__dict__.values() | ||
| if (isinstance(unit, UnitBase) and not isinstance(unit, PrefixUnit)) |
There was a problem hiding this comment.
Prefix units like u.THz are also excluded from the roundtrip test although it seems to work for the default format:
>>> isinstance(u.THz, u.PrefixUnit)
True
>>> u.THz.decompose().to_string()
'1e+12 / s'
>>> u.Unit(u.THz.decompose().to_string())
Unit("1e+12 / s")The issue seems to be with the decompose, the scale cannot be parsed back for console and unicode format. Without it works fine:
>>> u.Unit(u.THz.to_string(format="console"))
Unit("THz")
>>> u.Unit(u.THz.to_string(format="unicode"))
Unit("THz")The question would be if that is a common enough usecase with first decomposing and then roundtripping?
There was a problem hiding this comment.
For now, I guess it is a known limitation...
|
@mhvk - is there any chance you could review this today? I'd like to make a decision on whether to merge this in before branching v8.0.x or postponing to 8.1.0 |
- Forward `kwargs` in `check_roundtrip` - Add `test_composite_roundtrip` to test the roundtrip of a composite unit for different fraction and output formats
…tions they can be parsed back
744d36b to
59fc7f2
Compare
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
With possible scales not round-tripping, I would tend to make the comment more leaning towards not recommending using the format for anything that could be read in later; see suggestion in-line.
Note that it may be fairly straightforward to adjust the general parser such that it understands scale factors, but best not attempt this in this PR (or re-milestone to 8.1).
| [ | ||
| unit | ||
| for unit in u.__dict__.values() | ||
| if (isinstance(unit, UnitBase) and not isinstance(unit, PrefixUnit)) |
There was a problem hiding this comment.
For now, I guess it is a known limitation...
| ------ | ||
| s cm^2 | ||
|
|
||
| The ``"console"`` format can also be parsed back to a |Unit| object, but |
There was a problem hiding this comment.
I think I'd tend to write a bit more with the suggestion that it should not be used for parsing. For instance
The ``"console"`` format is not meant to be parsed back to |Unit| objects, even
though it does generally work as long as no scale factors are present and one
does not use ``fraction="multiline"``.
I don't think there is a need to give an example unless we manage to make the non-multiline case always work.
There was a problem hiding this comment.
Changed to your text and removed the example.
|
@mhvk just to check, are you agreeing with this going in 8.0.0 aside from the comment on the docs above? |
|
Yes, I think the added test is a good one; we might as well check that round-tripping continues to work (and there is no reason for it not to!). I just wanted not to explicitly encourage using the format for storage, say, if the roundtripping is not yet perfect. |
|
Ok so to confirm, can we merge this? (Branching shortly) |
dhomeier
left a comment
dhomeier
left a comment
There was a problem hiding this comment.
Agreed that the special cases of decomposition and long prefixes may be very uncommon, but relevant enough not to recommend this in general.
Also adding the respective functionality may still be worth the effort, but constitute new functionality IMO. While this PR as is could be considered a bugfix for inaccurate documentation, so can well go in for 8.0.0 (with or without the extra snippet).
|
I'll apply @dhomeier's suggestion and will then merge immediately since there's no reason (famous last words) those changes should do anything to the CI |
Co-authored-by: Derek Homeier <[email protected]>
|
This broke RTD. Please check CI carefully before merge. |
introduced by astropy#19571
|
I folded the fix into #19664 |
* Remove redundant elif in fast_converters.pyx * DOC: Fix formatting introduced by #19571
|
Thank you everyone for the helpful reviews to make this PR successful. |
Description
This pull request just moves the
consoleandunicodesections in the Built-In Formats section ofUnits and Quantities/String Representations of Units and Quantitiesand adds a test. Previously they both were listed as able to write, but not read, so not supporting a round trip likeu.Unit(unit.to_string(format="console")).unicodeseems to be fixed since #11827 (thanks @dhomeier), but forconsoleI did not find the PR on the spot, nevertheless it seems to work since the added test works.Fixes #