Use quantity to value by mhvk · Pull Request #6137 · astropy/astropy

mhvk · 2017-05-30T17:19:24Z

saimn · 2017-05-30T17:41:55Z

+    ``to_value(unit)`` method, since this is somewhat faster. Any subclasses
+    that overwrote ``.to``, should also overwrite ``.tovalue`` (or
+    possibly just the priviate ``._to_value`` method.  (If you did this,
+    please let us know what was lacking that made this necessary!). [#6136]


typos: tovalue → to_value, priviate → private

Thanks! Since travis and appveyor hadn't even started yet, I cancelled those builds and fixed this immediately

taldcroft

A couple of questions, otherwise looks good to me.

taldcroft · 2017-05-30T17:31:44Z


+  - Internally, astropy replaced use of ``.to(unit).value`` with the new
+    ``to_value(unit)`` method, since this is somewhat faster. Any subclasses
+    that overwrote ``.to``, should also overwrite ``.tovalue`` (or


typo .to_value

taldcroft · 2017-05-30T17:34:33Z

        raise ValueError("Size Error: Arrays will be {}.  Use "
                         "allow_huge=True to override this exception."
-                         .format(human_file_size(array_size_C.to(u.byte).value)))
+                         .format(human_file_size(array_size_C)))


I take it this is intended?

Yes, human_file_size understands Quantity, so no need to convert here. I think this was just missed when human_file_size was changed.

taldcroft · 2017-05-30T17:47:29Z

+            # Possibly scaled unit any quantity-likes should be converted to
            _unit = u.CompositeUnit(getattr(self, 'unit', 1.), [u.day], [1])
-            val1 = val1.to(_unit).value
+            val1 = u.Quantity(val1, copy=False).to_value(_unit)


I'm sure there is a reason, but why did you change in this way?

At first, I just changed to to_value, but this failed, since one of the tests uses Column (which has a to method, but not a to_value). When I noticed that, I felt it was actually better not to assume having a unit goes with having to method either, but rather just let the Quantity initializer deal with different objects that have a .unit attribute.

OK, now I understand. I've never liked duck-typing by attribute names.

taldcroft · 2017-05-30T18:50:28Z

Question -- there was one tested instance where a both a Column and Quantity were acceptable inputs to a method. That's because Column behaves (a little bit) like a Quantity. So is it possible in the other 204 changes that this could happen as well? Does Column need to acquire a to_value method just in case??

mhvk · 2017-05-30T19:37:05Z

        val = self._match_shape(val)
        if hasattr(val, 'to'):  # Matches Quantity but also TimeDelta.
-            val = val.to(u.second).value
+            val = val.to_value(u.second)


Good call on checking for other classes. This one is wrong (and the other setter too). Will correct.

mhvk · 2017-05-30T20:02:01Z

Yes, good point. I went through the whole list again, and did find two instances where I should not have changed it: in the setters for delta_ut1_utc and delta_tdb_tt (though it was a match to TimeDelta rather than Column). Turns out those were not tested and failed even in current master. Anyway, this solves a small bug while we're at it!

mhvk · 2017-05-30T20:08:39Z

p.s. The other changes are all in places where it is known that we're dealing with a Quantity(large majority; test cases, coordinate conversions, etc.), or where the docstring explicitly states that a Quantity is expected (very few).

mhvk · 2017-05-31T19:59:50Z

I think this is ready to go in -- tests pass now. Any objections?

taldcroft · 2017-05-31T20:24:24Z

So my only outstanding question is whether we give Column a to_value method that just returns self.quantity.to_value(unit, equivalencies)?

In essence allow astropy and other developers to write code that treats Column and Quantity as kindof-duck-like and use the faster/nicer to_value method.

This would allow undoing 167082e etc., and in some sense then the tests would already be written. 😄

astrofrog · 2017-06-15T17:17:33Z

@mhvk - do you plan to wrap this up for 2.0?

taldcroft · 2017-06-15T18:47:04Z

I don't think that the issue of a Column.to_value method is a lien on this PR getting merged. No existing code will break and the only implication is that if other (non-astropy-core) code migrates to the to_value method then it won't work on Column objects. I think we can address this for 3.0.

So, I'm 👍 on merge (of course rebase is needed).

mhvk · 2017-06-15T21:40:27Z

I was indeed not planning to add anything to here; we can decide later whether it is useful for Column and TimeDelta to have to_value too (see #6148).

Anyway, now rebased.

bsipocz · 2017-06-15T21:53:44Z

    assert '...' in bigarrangle._repr_latex_()


+def test_rotation_matrix_deprecation():


@mhvk - your rebase must have gone wrong, these tests were removed in #6170

Ah, duh, that was a conflict and I did the wrong thing. Will fix...

No worries. I see thumbs up above, if this is good to go once given the all green by CI, please add the merge-when-ci-passes label.

bsipocz · 2017-06-15T21:54:28Z

Cancelling travis and appveyor as this has genuine failures, see comment and the CircleCI log.

astrobot · 2017-06-16T01:28:39Z

@bsipocz - thanks for merging this! However, I noticed the following issue with this pull request:

Changelog entry not present (or pull request number missing) and neither the Affects-dev nor the no-changelog-entry-needed label are set

Would it be possible to fix this? Thanks!

If you believe the above to be incorrect (which I - @astrobot - very much doubt) you can ping @astrofrog

pllim · 2017-06-16T01:30:45Z

Just for the record, change log is there but not this PR number.

bsipocz · 2017-06-16T01:30:59Z

+    ``to_value(unit)`` method, since this is somewhat faster. Any subclasses
+    that overwrote ``.to``, should also overwrite ``.to_value`` (or
+    possibly just the private ``._to_value`` method.  (If you did this,
+    please let us know what was lacking that made this necessary!). [#6136]


Typo in PR number that astrobot noted. I'll fix it before release.

It is actually the issue number this PR fixes, so not strictly a typo. 😄

Then the issue is the missing PR number. Neither the bot, nor the release script care about issue numbers, but about the PR numbers :)

Sorry to have missed that...

dhomeier · 2017-07-06T19:54:05Z

This did break some code of mine where I called spectral_density(wl) with a table Column wl.
It's easily enough fixed with a Quantity(wl), but the exception is a bit opaque, being raised three call levels down...

I realise that spectral_density has required a Quantity as input for some time, but perhaps it (and other function that have switched to the to_value method) should check the input type in the same way as for UnitBase.

mhvk added convolution coordinates cosmology time units visualization visualization.wcsaxes nddata Performance Refactoring utils wcs labels May 30, 2017

mhvk added this to the v2.0.0 milestone May 30, 2017

This was referenced May 30, 2017

Replace instances of .to(unit).value with .to_value(unit) #6136

Closed

Shortcut for Quantity q.to(unit).value? #6125

Closed

bsipocz added the no-changelog-entry-needed label May 30, 2017

saimn reviewed May 30, 2017

View reviewed changes

mhvk force-pushed the use-quantity-to-value branch from 6abb25a to fde260c Compare May 30, 2017 17:48

bsipocz removed the no-changelog-entry-needed label May 30, 2017

taldcroft reviewed May 30, 2017

View reviewed changes

mhvk commented May 30, 2017

View reviewed changes

mhvk mentioned this pull request May 30, 2017

Table unicode sandwich - make 'S' type useful in Python 3 #5700

Merged

mhvk force-pushed the use-quantity-to-value branch from aeead99 to acdfd67 Compare May 31, 2017 17:38

mhvk added the Ready-for-final-review label May 31, 2017

mhvk added the Affects-release label May 31, 2017

mhvk added 14 commits June 15, 2017 23:36

Use to_value in documentation.

17b3c9a

Use to_value in nddata.

a64f411

Use to_value in utils.

a2d76d0

Use to_value in cosmology.

5f43c87

Use to_value in units.

1adfc40

Use to_value in visualization.

ddc89e7

Use to_value in wcs.

2811e8f

Use to_value in time.

a6917a7

Use to_value in convolution.

57fd618

Use to_value in coordinates.

9122e1e

Clarify difference between to_value() and to().value.

556a0d9

Note the effective API change for Quantity subclasses.

14cb26f

Remove unwarranted change to to_value in Time.

b058209

Add test case for set_delta_* with TimeDelta argument.

15d4302

mhvk force-pushed the use-quantity-to-value branch from acdfd67 to 6730efa Compare June 15, 2017 21:38

bsipocz reviewed Jun 15, 2017

View reviewed changes

Post-rebase fix to changelog and test_angles

d34e9f3

mhvk force-pushed the use-quantity-to-value branch from 6730efa to d34e9f3 Compare June 15, 2017 22:10

mhvk added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Jun 15, 2017

bsipocz merged commit 0f0d40b into astropy:master Jun 16, 2017

bsipocz reviewed Jun 16, 2017

View reviewed changes

mhvk deleted the use-quantity-to-value branch June 16, 2017 05:55

pllim mentioned this pull request Jun 27, 2017

Semi-random test failure of QTable operations #6291

Closed

		assert '...' in bigarrangle._repr_latex_()


		def test_rotation_matrix_deprecation():

Conversation

mhvk commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taldcroft commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 30, 2017

Uh oh!

mhvk commented May 30, 2017

Uh oh!

mhvk commented May 31, 2017

Uh oh!

taldcroft commented May 31, 2017

Uh oh!

astrofrog commented Jun 15, 2017

Uh oh!

taldcroft commented Jun 15, 2017

Uh oh!

mhvk commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrobot commented Jun 16, 2017

Uh oh!

pllim commented Jun 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhomeier commented Jul 6, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

bsipocz commented Jun 15, 2017 •

edited

Loading