.to().value -> .to_value speedup

[nstarman]

.to().value is slower than to_value() since .to makes a copy and then returns a view of the object's value, while to_value performs the optimized set of operations, which is situation dependent, to accomplish the same thing. At worst they are the same speed.

[codecov]

Codecov Report

Merging #428 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #428   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files         168      168           
  Lines       25332    25332           
=======================================
  Hits        25287    25287           
  Misses         45       45           

Impacted Files	Coverage Δ
galpy/actionAngle/actionAngle.py	100.00% <100.00%> (ø)
galpy/actionAngle/actionAngleHarmonic.py	100.00% <100.00%> (ø)
galpy/actionAngle/actionAngleHarmonicInverse.py	100.00% <100.00%> (ø)
galpy/actionAngle/actionAngleIsochrone.py	100.00% <100.00%> (ø)
galpy/actionAngle/actionAngleIsochroneApprox.py	100.00% <100.00%> (ø)
galpy/actionAngle/actionAngleIsochroneInverse.py	100.00% <100.00%> (ø)
galpy/actionAngle/actionAngleStaeckel.py	98.91% <100.00%> (ø)
galpy/actionAngle/actionAngleStaeckelGrid.py	99.70% <100.00%> (ø)
galpy/df/df.py	100.00% <100.00%> (ø)
galpy/df/diskdf.py	100.00% <100.00%> (ø)
... and 52 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1617006...281da71. Read the comment docs.

[jobovy]

Thanks! Yeah, I didn't know about to_value when I wrote most of that code... Do you know when it was added? Just wondering what the chances are that people don't have access to it for their astropy version (maybe it's always been there?).

There's a test that fails, haven't looked why.

I'm currently in the process of merging a rename of some files in galpy.util, which I will merge first. So some of these changes will have to be edited then.

[nstarman]

Ok. The failures are a bit strange. I'll have to look into that. I can just do a merge from master when the other PR is done.

[jobovy]

Okay, I've merged it now.

Note that I'm also planning on centralizing how quantities are parsed by just defining parsers for different units in galpy.util.conversion (so you can do a= conversion.parse_distance(a,ro=ro,vo=vo) rather than writing out the whole if a is a quantity: a= a.to_value(units.kpc)/ro)). See an example in the new galpy.potential.Potential.zvc. So that would remove most of the current .to(...).value statements. It may be best to hold off on this PR until then.

[nstarman]

Fair enough. I just resolved conflicts. I'll hold off further testing until the unit parsing PR is done.

[nstarman]

Do you know when it was added? Just wondering what the chances are that people don't have access to it for their astropy version (maybe it's always been there?).

I don't know when it was added. I just looked at their docs and it seems to be there from at least 2.0 on, maybe earlier. That could be a backport though. It should be there for python 2.7.

[jobovy]

Okay, thanks, so it seems like to_value has probably existed for a long time and I just didn't know about it (happens a lot, I didn't know numpy.geomspace existed until a few months ago...).

I'm not about to do the parse PR, so we could just merge this in the mean time if the tests pass.

[nstarman]

numpy.geomspace existed until a few months ago...)

Thanks! I didn't know about that function. looks useful!

[nstarman]

The tests aren't passing and the errors are super strange. The one I looked at more deeply seems to have problems with Orbit.phi, which doesn't make any sense since it calls _call_internal which only has one change: t.to_value(u.Gyr)

[jobovy]

Okay, I think I fixed the issue, which seems to stem from the fact that running to_value on an array somehow creates a reference to the original array, such that if you later change the unitless array, it also modifies the original array. E.g.

def check(r):
    r= r.to_value(u.kpc) 
    r*= -1. 
    print(r)
r= 4*u.kpc
check(r)
# -4.0
print(r)
# <Quantity 4. kpc>, as expected

but

r= 4*u.kpc*numpy.ones(3)
check(r)
# [-4. -4. -4.]
print(r)
# <Quantity [-4., -4., -4.] kpc>, changed from original

This seems like unexpected behavior to me, because once I think I'm dealing with floats, I would think that they would no longer communicate with the original Quantity array, especially because

isinstance(r.to_value(u.kpc),u.Quantity)
# False

So now I'm a little reluctant to switch to using to_value, because this may lead to unexpected behavior in the future.

[nstarman]

I see. I’m looking at the documentation for `to_value` and I see that it returns a view object if the unit to which to convert is the same as the current unit. This is definitely a speedup, but does have the drawback you mentioned.

…

On Aug 24, 2020, at 11:59, Jo Bovy ***@***.***> wrote: Okay, I think I fixed the issue, which seems to stem from the fact that running to_value on an array somehow creates a reference to the original array, such that if you later change the unitless array, it also modifies the original array. E.g. def check(r): r= r.to_value(u.kpc) r*= -1. print(r) r= 4*u.kpc check(r) # -4.0 print(r) # <Quantity 4. kpc>, as expected but r= 4*u.kpc*numpy.ones(3) check(r) # [-4. -4. -4.] print(r) # <Quantity [-4., -4., -4.] kpc>, changed from original This seems like unexpected behavior to me, because once I think I'm dealing with floats, I would think that they would no longer communicate with the original Quantity array, especially because isinstance(r.to_value(u.kpc),u.Quantity) # False So now I'm a little reluctant to switch to using to_value, because this may lead to unexpected behavior in the future. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#428 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACEI7EKBZG7EQLDJH2NFHM3SCKE47ANCNFSM4QI36TWA>.

[jobovy]

I guess one could use

r.to_value(u.kpc).astype('float')

but I don't know whether that defeats the speed-up purpose of using to_value in the first place?

[nstarman]

That would work, but I do think it is effectively .to. Perhaps internal functions should use to_value, but user-facing ones use .to().value, to ensure immutability of state.
Because that in-place operator on a view object is pretty scary.

[nstarman]

Why does the codecov look so terrible? I'm pouring over all the diffs and there doesn't seem to be any coverage change.

[jobovy]

The Codecov stats were off, because a few of the jobs didn't report to Codecov (the reporting is quite flaky, often at least one job will just fail to run the reporting script). I re-ran them and now all looks good.

There isn't really any internal use of to().value, all of it is to process user input.

[jobovy]

I made the switch to using a centralized set of Quantity parsers as much as possible in #430. With that in place and the danger of using .to_value (because it doesn't copy), I don't think this PR is still relevant?

[nstarman]

I agree.

[jobovy]

Okay, I'll close this one then. But thanks for giving me the push to simplify the Quantity parsing!