Update physical type implementation by namurphy · Pull Request #11625 · astropy/astropy

namurphy · 2021-04-22T17:26:32Z

Description

This pull request is to clean up the implementation of PhysicalType in a few places. The changes I'm planning to make are:

Add 'velocity' as a physical type name along with 'speed'
Remove 'sustained displacement' from 'absement' because the former is more like a definition than a name
Make string methods work with PhysicalType instances (Make PhysicalType instances support string methods with deprecation warnings #11617)
Remove string with set notation from PhysicalType.__str__

~~The changes to __str__ would affect #11618, or perhaps be made in #11618 instead.~~

EDIT: This is a follow-up of #11204 , fix #11617

github-actions · 2021-04-22T17:27:49Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

- Added 'velocity' as a physical type name along with 'speed' - Removed 'sustained displacement' from 'absement' because the former is more like a definition than a name

Because the physical_type attribute of a unit was a str before astropy#11204, there may be code that depends on those str methods for the physical_type attribute. This approach was adapted from code suggested in astropy#11204 after it was merged. Co-authored-by: Marten van Kerkwijk <mhvk@astro.utoronto.ca>

I made it return 'energy density, pressure, stress' rather than using set notation. I tentatively chose commas rather than slashes for readability.

nstarman

Excepting a changelog, LGTM!

nstarman · 2021-04-25T17:52:53Z

+    "method, expected",
+    [
+        ("title", 'Length'),
+        ("isalpha", True),
+        ("isnumeric", False),
+        ("upper", 'LENGTH')
+    ],
+)


My preference is also for black but https://docs.astropy.org/en/stable/development/codeguide.html?highlight=black#coding-style-conventions.

black isn't encouraged much in this repo, as it conflicts with existing style some times. The perception might change in the future, so who knows.

I changed the style a bit in response, but any of the maintainers should please feel free to suggest and accept stylistic changes.

The perception might change in the future, so who knows.

I've been itching to do >>> black astropy and git push for years now. 🤣

No, please, no!!

namurphy · 2021-04-25T17:58:38Z

Ready for review! Some questions...

When there are multiple physical types names, what should the PhysicalType.__str__ separator character be? Two of the main possibilities are a , or a /. That would give 'speed, velocity' or 'speed/velocity'.

I chose , for now because it's more readable to me than / when there are physical type names with multiple words, and / makes me think division. The advantage of / is that it would be a valid input for PhysicalType for backwards compatibility, but currently not for get_physical_type which is intended to be one of the two main ways to access physical types.

If we want to get more readable/fancy, we could even do something more complicated like 'speed or velocity' or 'energy density, pressure, or stress'.
Am I handling the deprecation warning appropriately? In particular, I'm not specifying the version of Astropy it'll be removed in.

[slightly edited for clarity]

nstarman · 2021-04-25T18:11:02Z

When there are multiple physical types names, what should the PhysicalType.__str__ separator character be? Two of the main possibilities are a , or a /. That would give 'speed, velocity' or 'speed/velocity'.
I chose , for now because it's more readable to me when there are physical type names with multiple words than /, and / makes me think division. The advantage of / is that it would be a valid input for PhysicalType for backwards compatibility, but currently not for get_physical_type which is intended to be one of the two main ways to access physical types.

I prefer "," (but actually not visually). This class is new behavior and it seems reasonable to prioritize get_physical_type.

If we want to get more readable/fancy, we could even do something more complicated like 'speed or velocity' or 'energy density, pressure, or stress'.

Looks nice. But it doesn't feel necessary. I know in formal logic "or" is considered inclusive, but that's not true colloquially. Identifying physical types by their units makes it impossible to differentiate between energy density and pressure, so it feels wrong to use "or". But that's just IMO.

Am I handling the deprecation warning appropriately? In particular, I'm not specifying the version of Astropy it'll be removed in.

Do we even want to set a hard deadline? This backwards compatibility is to give affiliate packages time and reason to switch. I think it's better to have this be unspecified, have someone stumble on it in a few years, say "Oh yeah!", and ask around if anyone's still using it. @pllim, you have one such affiliate package. Thoughts?

nstarman · 2021-04-26T18:02:41Z

@mhvk LGTM

pllim · 2021-04-26T20:53:23Z

This deprecation comes a bit too late to the release cycle than I like... Doesn't give downstream maintainers much time to catch up before branching happens. Still, hopefully minor enough that it won't cause too much trouble, and better to get this in before #11204 is released. 🤞

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

namurphy · 2021-04-27T01:26:48Z

This deprecation comes a bit too late to the release cycle than I like...

I had intended to get #11204 finished two months earlier than I did, but pandemic era brain blah got the better of me...

Still, hopefully minor enough that it won't cause too much trouble, and better to get this in before #11204 is released. crossed_fingers

I just did a search of GitHub, and I found only one usage of a string method on a physical_type attribute of a unit before GitHub's search interface started to make my brain hurt. With this PR, PhysicalType should be duck typing previous string behavior pretty well. I hope. 😸

mhvk

Some small bits, but one slightly larger question for __str__, on what the separator should be.

mhvk · 2021-04-27T18:20:04Z

+                "from PhysicalType instances is deprecated since 4.3 and will be removed in a "
+                "subsequent release.")
+            warnings.warn(warning_message, AstropyDeprecationWarning)
+            self_str_attr = getattr(str(self), attr, None)


No need for the ", None" any more if attr in dir(str), right?

Note that personally I'd prefer to stick with keeping this on top - if we enter __getattr__, regular attribute access has failed already so it doesn't really matter if it is slightly slower. And I'm just not 100% sure that dir(str) and hasattr(str(self), attr) are really completely equivalent...

mhvk · 2021-04-27T18:20:33Z

+            self_str_attr = getattr(str(self), attr, None)
+            return self_str_attr
+        else:
+            raise AttributeError(


Just do super().__getattribute__(attr) so you get the standard error message.

🤣. @namurphy, apologies for suggesting otherwise.

mhvk · 2021-04-27T18:22:17Z

-            return self._physical_type_list[0]
-        else:
-            return self._name_string_as_ordered_set()
+        return ", ".join(self._physical_type_list)


If we're dropping the braces, should we make this "/".join(...) so that in principle it can be read back?

Is there any expectation that __str__ can be read back? I thought that's what __repr__ is for

Agreed, just that we accept / on input, so it seems perhaps more logical to re-use that. I also like that it is commonly used to mean "or", while with the , the braces helped to make that clear.

Clarity and round-tripping are definitely good things. I don't have a strong opinion whether "/" XOR "," is superior as they have a similar set of advantages and disadvantages, depending on use case.

It would be worthwhile to have operations like u.Pa.physical_type == str(u.Pa.physical_type) work as expected, so I'll change it to /.

We'd have to do a bit more to make it a full round trip, since the / separated physical types are allowed in def_physical_type and PhysicalType and with operators like ==, but not in get_physical_type. This was necessary for the former three cases since there was a previously a str physical type named "momentum/impulse" and we wanted to maintain backward compatibility, but this wasn't a concern for get_physical_type since it previously returned "momentum/impulse" instead accepting it as an argument. Adding the full round trip is out of scope of this PR but it's something that could be added in later.

namurphy · 2021-04-28T20:36:17Z

Closes #11617.

pllim · 2021-04-28T21:00:39Z

Re: #11625 (comment) -- @namurphy , the auto-close feature only kicks in if you state that in the original post. I have updated the original post for you. FYI.

mhvk

This looks all great to me now! Let's get this in!

dhomeier · 2021-06-23T15:53:53Z

Thanks for providing a more convenient access here. It seems some operations like
if cm_unit.physical_type in ('length', 'frequency'):
still need explicit conversion to str(), or would there be a better alternative?

nstarman · 2021-06-23T16:09:18Z

u.cm.physical_type in ('length', 'frequency') returns True for me. (Python 3.9, Mac, Astropy 5.0dev)

It should work because the tuple.__contains__ does equality testing on each element and u.cm.physical_type == 'length' is True because PhysicalType does a string conversion internally.

dhomeier · 2021-06-23T16:25:24Z

OK, that does work on 4.3rc1, and even
u.eV.physical_type in ('length', 'freq', 'energy')
evaluates as True. Thanks!
So it's only the inverse lookup for a partial string
'spectral flux' in u.Jy.physical_type
that needs being taken care of...

nstarman · 2021-06-23T16:35:17Z

I guess PhysicalType needs to implement __contains__

mhvk · 2021-06-23T16:51:38Z

I'm -0 on __contains__ - may better for that case let people do 'spectral flux' in str(physical_type)? While adding __contains__ means we would improve backward compatibility, I'm not sure one should aim for matching on substrings generally.

github-actions Bot added the units label Apr 22, 2021

namurphy mentioned this pull request Apr 22, 2021

Add physical module to units index, and include list of types #11618

Merged

namurphy force-pushed the physical-type-updates branch from 97de10a to 8df8da0 Compare April 23, 2021 02:00

nstarman reviewed Apr 25, 2021

View reviewed changes

Comment thread astropy/units/physical.py Outdated

namurphy and others added 5 commits April 25, 2021 13:17

Update physical types for speed & absement

747ca26

- Added 'velocity' as a physical type name along with 'speed' - Removed 'sustained displacement' from 'absement' because the former is more like a definition than a name

Change PhysicalType.__str__

a3c6d50

I made it return 'energy density, pressure, stress' rather than using set notation. I tentatively chose commas rather than slashes for readability.

Update expected error message

7fd14f1

Remove unnecessary clause in physical type if statement

a77e1d4

namurphy force-pushed the physical-type-updates branch from 5287364 to a77e1d4 Compare April 25, 2021 17:17

namurphy marked this pull request as ready for review April 25, 2021 17:24

nstarman reviewed Apr 25, 2021

View reviewed changes

Comment thread astropy/units/physical.py Outdated

nstarman approved these changes Apr 25, 2021

View reviewed changes

Call AttributeError directly rather than through super()

96d9230

namurphy force-pushed the physical-type-updates branch from 5b22b1b to 96d9230 Compare April 25, 2021 18:40

pllim added this to the v4.3 milestone Apr 26, 2021

pllim added Affects-dev PRs and issues that do not impact an existing Astropy release no-changelog-entry-needed labels Apr 26, 2021

pllim reviewed Apr 26, 2021

View reviewed changes

Comment thread astropy/units/physical.py Outdated

pllim reviewed Apr 26, 2021

View reviewed changes

Comment thread astropy/units/tests/test_physical.py Outdated

namurphy and others added 3 commits April 26, 2021 18:28

Apply suggestions from physical type code review

ef86300

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

Fix mistake in physical type docs

0c50743

Update coding style

bf24b4a

mhvk reviewed Apr 27, 2021

View reviewed changes

namurphy added 5 commits April 27, 2021 21:55

Change implementation for using str attrs with PhysicalType

3235d2e

Change separation character for PhysicalType.__str__

e52c142

Update tests with new separation character

8b353cb

Fix getattr statement in PhysicalType

0e988f0

Update error message in test

6237c45

namurphy requested a review from mhvk April 28, 2021 18:11

pllim mentioned this pull request Apr 28, 2021

Make PhysicalType instances support string methods with deprecation warnings #11617

Closed

mhvk approved these changes Apr 29, 2021

View reviewed changes

mhvk merged commit 0cebd7c into astropy:main Apr 29, 2021

namurphy deleted the physical-type-updates branch June 23, 2021 17:08

dhomeier mentioned this pull request Jun 23, 2021

Handle new PhysicalType class in spectrum_from_column_mapping astropy/specutils#833

Merged

eerovaher mentioned this pull request Oct 20, 2023

Remove deprecated functionality from units #15514

Merged

1 task

Uh oh!

Uh oh!

Conversation

namurphy commented Apr 22, 2021 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Apr 22, 2021

Uh oh!

Uh oh!

Uh oh!

nstarman 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!

namurphy Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

namurphy commented Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nstarman commented Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nstarman commented Apr 26, 2021

Uh oh!

Uh oh!

Uh oh!

pllim commented Apr 26, 2021

Uh oh!

namurphy commented Apr 27, 2021

Uh oh!

mhvk 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!

nstarman Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namurphy commented Apr 28, 2021

Uh oh!

pllim commented Apr 28, 2021

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

dhomeier commented Jun 23, 2021

Uh oh!

nstarman commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhomeier commented Jun 23, 2021

Uh oh!

nstarman commented Jun 23, 2021

Uh oh!

mhvk commented Jun 23, 2021

Uh oh!

namurphy commented Apr 22, 2021 •

edited by pllim

Loading

namurphy Apr 27, 2021 •

edited

Loading

namurphy commented Apr 25, 2021 •

edited

Loading

nstarman commented Apr 25, 2021 •

edited

Loading

nstarman Apr 27, 2021 •

edited

Loading

nstarman commented Jun 23, 2021 •

edited

Loading