Repository navigation
Fix units to be able to compose from equivalent units that have prefixes - #6957
Conversation
|
Hi there @fastakash 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
|
Hi @bsipocz can you please add the label work-in-progress? I initially thought my work was fine but now see that a failing unit-test is because of the change I made. I will investigate further. Sorry for the PR being premature |
|
The label "work in progress" may now be removed. Hoping for a review/comment next! |
bsipocz
changed the title
|
@fastakash - I've changed the title of your PR to be more informative about its content (as probably none of us knows the issues by their number). |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
@fastakash - thanks very much for addressing this long-standing issue! Your solution is good and my comments are about details of the tests, and ensuring the new behaviour is documented.
| (u.AA * u.erg) ** 9 | ||
|
|
||
|
|
||
| def test_compose(): |
There was a problem hiding this comment.
The tests seem somewhat scattered already, but maybe try to keep this closer to another similar set of tests; e.g., just before test_compose_issue_579 and then name your functiontest_compose_prefix_unit.
| # if the specified units list explicitly contains prefix units, | ||
| # include_prefix_units should be turned on. Ex: units=[u.kpc] | ||
| if include_prefix_units is None: | ||
| include_prefix_units = isinstance(units, (list, tuple)) |
There was a problem hiding this comment.
We should be sure to also document this, i.e., in the description of include_prefix_units. Maybe just
Default is `True` if a sequence is passed in to ``units``, `False` otherwise.
|
|
||
|
|
||
| def test_compose(): | ||
| x = u.m.compose(units=(u.m)) |
There was a problem hiding this comment.
I think this is sufficiently tested elsewhere, so I would instead replace it with something else that uses a tuple (but then add the trailing ,!). Also, maybe make the test a bit more realistic by adding multiple possible units. E.g.,
x = (u.km/u.s).compose(units=(u.pc, u.Myr))
assert x[0].bases == [u.pc, u.Myr]
assert_allclose(x[0].scale, 1.0227121650537077)
| x = u.m.compose(units=[u.km]) | ||
| assert x[0].bases[0] is u.km | ||
| assert x[0].scale == 0.001 | ||
|
|
There was a problem hiding this comment.
Of course, also important to test whether things fail when include_prefix_units=False:
with raises(u.UnitsError):
(u.km/u.s).compose(units=(u.pc, u.Myr), include_prefix_units=False)
| ^^^^^^^^^^^^^ | ||
|
|
||
| - When a prefix unit is specified, include_prefix_units should be automatic | ||
| [#4104, #6957] |
There was a problem hiding this comment.
We usually just specify the PR, not the original issue.
|
@mhvk Thanks for patiently explaining everything to me! I hope I addressed all your feedback. I will be more careful with reviewing all the existing unittests in the future. I know this was a small task that you could have probably finished yourself in minutes! I hope to pick up more and bigger tasks, especially in the summer. |
|
Hi @mhvk Thanks again for your help. Do you think my updates are ok? If you are busy, please ignore this message! I will start looking at other issues.. thanks |
| include_prefix_units : bool, optional | ||
| Default is `True` if a sequence is passed in to ``units``, | ||
| `False` otherwise. | ||
| When `True`, include prefixed units in the result. |
There was a problem hiding this comment.
THis got a bit mangled. Could you change it to
When `True`, include prefixed units in the result.
Default is `True` if a sequence is passed in to ``units``,
`False` otherwise.
|
@fastakash - apart from the very minor comment about the docstring, this all looks good. If you change that one, add |
if the specified units list explicitly contains prefix units, include_prefix_units should be turned on. Ex: units=[u.kpc] if the units is a module, ex: cgs, the behaviour is unchanged
65128d6 to
2a24ec2
Compare
|
@fastakash - I realized the changelog was for 3.0, but this will have to be in 3.1, so I went ahead and moved it (rebasing to be sure I picked up the new changelong entries). Otherwise, it all looks good, so I'll merge. Thanks! |
Fix #4104.
Made changes as suggested and added some tests also. My first commit to astropy and github! Took me a long time to figure out all the steps.