DEPR: deprecate passing redshift (`z`) as a keyword argument by neutrinoceros · Pull Request #16597 · astropy/astropy

neutrinoceros · 2024-06-20T15:54:02Z

Description

This is the continuation (and revival) of @nstarman's work in #15859. See that PR for motivation.

~~It is still a work in progress as I haven't applied my decorator anywhere yet. I'm opening as a draft to check for compilation and other installation errors on all platforms.~~
Close #15859

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

TODO:

pass existing CI
add tests (I already wrote some, just need to port them to this PR)
add a docstring (and any documentation required)
apply decorator instead of making arguments positional only directly
edit changelog

github-actions · 2024-06-20T15:54:28Z

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.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

github-actions · 2024-06-20T15:54:31Z

👋 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?

neutrinoceros · 2024-06-20T18:50:24Z

@nstarman I just realized that we could skip the deprecation cycle for private methods. Do you agree ?

nstarman · 2024-06-20T20:03:35Z

Yes! Only public functions.

neutrinoceros · 2024-06-21T14:00:14Z

I just included my test after I did some refactoring (probably a mistake) and now they are failing locally. I still pushed to see if the bug also shows in CI, but clearly this needs some more tinkering.

neutrinoceros · 2024-06-21T15:48:14Z

Almost there. I collapsed my branch after I made sure all lights were green. Before I undraft this, I'd like to give @nstarman priority for reviewing since this is based off his work and largely co-authored.

neutrinoceros

Leaving myself a comment from my phone.

nstarman · 2024-06-21T23:14:56Z

+    @deprecated_keywords("z", since="7.0")
    def scale_factor(self, z):


Haven't looked through the C-code yet, but does @deprecated_keywords map to the positions? As in, can this be made positional-only now? And then we remove the decorator later, to complete the deprecation?

It doesn't. Maybe it's possible but I don't know the C API well enough to see how it could be implemented.

Actually I do think it's feasible, but it's a trade-of for where we put complexity: it would certainly make the C extension more complex and I'm already pushing my own limit there so I'm not confident that I could build this feature without hurting robustness and readability. How about I give it a try as a follow up task ?

I think that's totally reasonable! And thanks again for taking the lead on this!
Having the parameters be positional-only at the undecorated Python function level is an improvement, but to the user there is no difference, so the PR as written is great 👍.

dully noted !

In hindsight and given the complaints this PR got for introducing a hard-to-maintain C extension, I'm not sure it's worth pushing it any further without a clear need to.

mhvk · 2024-06-22T06:49:07Z

I'm on holidays this week, but FWIW happy with the general idea.

nstarman

Thanks @neutrinoceros! This looks great. I would love it if someone else also reviewed the C-code, as I'm much less conversant in C-to-Python.

nstarman · 2024-06-24T16:10:03Z

+# F = TypeVar("F", bound=Callable[..., Any])
+F = Callable[..., Any]


I think you can do

Suggested change

# F = TypeVar("F", bound=Callable[..., Any])

F = Callable[..., Any]

_F = TypeVar("_F", bound=Callable[..., Any])

pllim · 2024-06-24T18:36:35Z

 from inspect import signature
+from typing import Any

+from ._signature_deprecations import _depr_kws_wrap


I am not familiar with cosmology code base, so please explain to me why deprecating z in cosmology needs C extension in utils. Thanks!

Ah, I just saw #15859 (comment) . Performance boost is always welcome but I am afraid this C code would also make the decorator hard to maintain going forward. I will put this on queue to review.

Oh, so this boost is only for a very specific case. And I feel like the new function (ignoring performance) duplicated something deprecated_renamed_argument can already do. I need to ponder.

Even if we decide to keep this as-is, license for https://pypi.org/project/positional-defaults/ would need to be included in https://github.com/astropy/astropy/tree/main/licenses

Also the new function is technically new API, so might also need a change log for utils.

And I feel like the new function (ignoring performance) duplicated something deprecated_renamed_argument can already do.

I don't think that's the case. deprecated_renamed_argument only works if a keyword argument is still allowed, albeit under a new name, while deprecated_keyword forces the user to stop using the argument as keyword.

I am afraid this C code would also make the decorator hard to maintain going forward
Also the new function is technically new API

Good points. We don't actually need to make it public API. Maybe we can keep it private for now and see where it goes ? If it's not used at all after this deprecation cycle is concluded we can just get rid of this C extension altogether ?

Even if we decide to keep this as-is, license for pypi.org/project/positional-defaults would need to be included in main/licenses

Thanks !

Good points. We don't actually need to make it public API

Agreed. Let's make it private. If @mhvk or @pllim prefers, we can also move it into cosmology. Though looking around Astropy I would recommend other maintainers avail themselves of this useful utility and consider making some required arguments positional-only.

mhvk · 2024-06-25T13:10:23Z

Definitely from the sidelines, but this seems an inordinate amount of extra code for what essentially is beautification of the code. After all, nothing is stopping people now from using a positional argument; all this does is stop people from using z=.... Is this worth it?

neutrinoceros · 2024-06-25T13:22:18Z

@nstarman do you want to reply here ?

nstarman · 2024-06-25T14:39:06Z

Is this worth it?

Yes! There are a few motivating factors behind this change:

Clarity. There are more concepts in math/physics than there are symbols. z is the common symbol used for redshift in Cosmology, but also for complex numbers, and now that ML has permeated cosmology also for latent spaces. It's often clear in context, both mathematically and in code, but it's worthwhile asking "what should I name my function argument?". redshift is very clearly a better variable / argument name as it's much less ambiguous, but it's also 8x more verbose. redshift also isn't perfect because cosmology functions are interested in the cosmological redshift, not doppler redshift. So cosmo_redshift would be a yet-more-clear function name. But that is 14x more verbose than z. Thankfully positional-only arguments mitigate this problem. Let's see PEP 570:

When designing APIs (application programming interfaces), library authors try to ensure correct and intended usage of an API. Without the ability to specify which parameters are positional-only, library authors must be careful when choosing appropriate parameter names. This care must be taken even for required parameters or when the parameters have no external semantic meaning for callers of the API.

All of the functions that are being updated take the redshift as their first (or first 2) required arguments. The meaning of the function is contextually clear even without the variable names (which are still visible even when positional-only and further clarify meaning). So given that z is only a decent variable name choice and that there are a large number of appropriate and clearer variable names, providing 100% clarity by making the argument positional-only is simply better API.

Downstream Impact: Minimal because almost everyone passes it in primarily by position. For the less-common cases there's a deprecation period. Any code just needs to be changed from (z=x, ...) to (x)`. I took a look at GH and didn't find any counterexamples to this easy transition.
Interoperability with other libraries. From point 1 I discussed how there are reasonable (and possibly better) variable names than z. Libraries that build upon astropy.cosmology even think so. SkyPy uses redshift as its variable name of choice. Making this argument positional-only is a better API choice for connecting to other libraries. They can name a variable whatever they want (or better yet make it positional) and people (once they are familiar with the change in Astropy) will not have any mental dissonance about redshift versus z because positional-only arguments are positional, not named. Per PEP 570:

Positional-only parameters give more control to library authors to better express the intended usage of an API and allows the API to evolve in a safe, backward-compatible way.

Yes plz.

Last, as more a general philosophical point. These methods were written before positional-only arguments were a thing in Python. So when written they had to be positional-or-keyword. Once PEP 570 was implemented it is good to look at the code and ask where PEP 570 is relevant and can improve our API. This is one such place. We can and should improve the API in ways big and small. The reason this PR is heavy is because we are also dedicated to long-term support and smooth transitions. This PR could be very lightweight if we didn't offer a performant deprecation period!

As a possible alternative, we could move the decorator into cosmology rather than utils. It can be private and temporary, added only for the length of the deprecation period.

pllim · 2024-06-25T14:47:28Z

Re: decorator -- If you think the performance is irrelevant outside of this narrow set of code, then it is best to keep it super private and then remove as @nstarman suggested.

Otherwise, I fear it could have package-wide implication. And I think the decorator speed-up is best done as a separate PR. And should be used more widely in other subpackages as well to be consistent.

neutrinoceros · 2024-06-26T08:12:18Z

Ok, let's make the decorator a private asset of astropy.cosmology. It doesn't even prevent promoting it to public API later if it ever grows in usage.

And I think the decorator speed-up is best done as a separate PR.

To be clear, are you suggesting I re-implement the decorator in Python only for the sake of delaying the more painful review ?
I don't mind re-implementing it in Python (I actually already have a mostly drop-in Python replacement), I'm just not sure that it actually helps; tests are already demonstrating how it works on the Python side. I don't mind going the longer way if requested, I just want to make sure that we all understand it is more work for all parties.

And should be used more widely in other subpackages as well to be consistent.

Sorry I don't get what you mean here.

pllim · 2024-06-26T12:35:09Z

To be clear, are you suggesting I re-implement the decorator in Python only for the sake of delaying the more painful review ?

Sorry I don't get what you mean here.

No, I mean, if you want to keep the C-extension and have it exposed via utils. We should also see if there are other similar deprecations that can use it as well.

let's make the decorator a private asset of astropy.cosmology

I am uneasy with this but it would be much less controversial.

Maybe we need to step back and rethink this whole thing. Let's look at this:

@deprecated_keywords("z", since="7.0")
def scale_factor(self, z):
    # ...

The signature has always suggests z is positional. It is not our problem that Python lets people also pass it in as z=. We never advertised it as such, did we? How many people out there in reality is actually passing in z=?

Can we just do this and be done with it?

def scale_factor(self, z, /):
    # ...

In the change log, we can say like "ooops you been using it in a way we didn't mean to support, you cannot do that anymore in 7.0" and that is probably okay because it is a major release and breaking change is allowed.

neutrinoceros · 2024-06-26T12:40:39Z

In the absence of a clear deprecation path, biting the bullet would make sense to me, but
now that there's one, I don't understand why we would risk antagonizing our users when we don't have to.

pllim · 2024-06-26T12:43:10Z

How many users are we antagonizing? If it is just one or two, I will take that risk. I will even open a PR in their repos.

pllim · 2024-06-26T12:44:44Z

Also if performance is really such a concern to the point you need C-extension for a decorator, biting bullet with / would be the most performant.

neutrinoceros · 2024-06-26T12:57:04Z

How many users are we antagonizing? If it is just one or two, I will take that risk. I will even open a PR in their repos.

No idea. Which is precisely why I'd err on the side of caution and avoid taking a risk. I also assume that a majority of our users wouldn't provide feedback or make their code public, so I would rather not create a precedent where we could have avoided immediate breakage for them and didn't.

mhvk · 2024-06-26T13:30:54Z

Again from the sidelines - while I would probably still not bother myself, the arguments for making it positional-only (eventually) are reasonable and now that one has done the work, it is perhaps a bit silly to not implement this. I think making it a cosmology-internal function is the way to go - if we need it elsewhere, we can still move it, but this way it is clear the whole stuff can go once the deprecation period is over. Indeed, would be good to open an issue now for 8.0(?) to remind us to remove all of it.

neutrinoceros · 2024-06-26T14:18:11Z

Here you go: #16614

nstarman · 2024-06-26T15:34:22Z

How many users are we antagonizing? If it is just one or two, I will take that risk. I will even open a PR in their repos.

Surprisingly a lot. https://github.com/search?q=distance%28z%3D&type=code is a quick search on just a small slice of our API.

We should also see if there are other similar deprecations that can use it as well.

Why don't we get this into cosmology, then ask other maintainers whether they're interested. From a very cursory search where I randomly click on a file and scroll through the first few functions with posional-or-keyword arguments, here's one example in almost every subpackage where positional-only arguments make sense.

"array" and/or "kernel" in

astropy/astropy/convolution/convolve.py

Lines 124 to 127 in 5519605

    
           def convolve( 
        
               array, 
        
               kernel, 
        
               boundary="fill",

"obj" in

astropy/astropy/coordinates/sky_coordinate.py

Line 511 in 5519605

def insert(self, obj, values, axis=0):

"data" in

astropy/astropy/io/registry/core.py

Line 343 in 5519605

def write(self, data, *args, format=None, **kwargs):

"inputs" in

astropy/astropy/modeling/core.py

Line 1076 in 5519605

def input_shape(self, inputs):

"val" in

astropy/astropy/nddata/bitmask.py

Line 61 in 5519605

def __new__(cls, val, doc=None):

"a" in

astropy/astropy/stats/histogram.py

Line 22 in 5519605

def calculate_bin_edges(a, bins=10, range=None, weights=None):

"table1", "table2" in

astropy/astropy/table/operations.py

Line 471 in 5519605

def setdiff(table1, table2, keys=None):

"other" in

astropy/astropy/time/core.py

Line 1251 in 5519605

def isclose(self, other, atol=None):

"array" in

astropy/astropy/timeseries/downsample.py

Line 16 in 5519605

def reduceat(array, indices, function):

etc.

pllim

Since I have been outvoted, I'll let Nathaniel or Marten to have one last look and merge. Just asking you to remove the diff from utils. Thanks!

Signed-off-by: nstarman <[email protected]>

neutrinoceros · 2024-06-26T15:47:36Z

done. I've also squashed my commits together (but left @nstarman's alone)

pllim · 2024-06-26T15:49:37Z

I just want to make @adrn aware of yet another utils module. (Inside joke...)

neutrinoceros · 2024-07-09T07:25:26Z

@nstarman is this ready to go ?

nstarman · 2024-07-10T17:41:52Z

Yes! I hadn't notice that you'd moved the C code into cosmology.
Let's get this in! 🎉

nstarman · 2024-07-10T17:46:02Z

Thanks @neutrinoceros ! This turned out nicely. And the timing arguments show the overhead is as small as can be reasonably accomplished for adding another function call to the stack.
Now that it's in cosmology, maybe it's worth inquiring whether any other sub-packages want to use this functionality.
@eerovaher, in coordinates, are there functions you want to have positional-only arguments? (e.g. #16597 (comment)).

eerovaher · 2024-07-10T18:57:22Z

I haven't thought about using positional-only function arguments in coordinates and I don't plan to think about that in the near future either. But maybe @adrn has ideas about this.

* DEPR: deprecate passing redshift (`z`) as a keyword argument Signed-off-by: nstarman <[email protected]> Co-authored-by: nstarman <[email protected]>

neutrinoceros mentioned this pull request Jun 20, 2024

redshift methods are positional-only #15859

Closed

github-actions Bot added the cosmology label Jun 20, 2024

github-actions Bot added the utils label Jun 20, 2024

nstarman added this to the v7.0.0 milestone Jun 20, 2024

neutrinoceros force-pushed the cosmology/depr/15859/deprecate_redshift_as_keyword branch from 88fbdb6 to d6efe8d Compare June 21, 2024 15:44

neutrinoceros commented Jun 21, 2024

View reviewed changes

Comment thread astropy/utils/tests/test_decorators.py Resolved

Comment thread astropy/utils/tests/test_decorators.py Resolved

Comment thread astropy/utils/tests/test_decorators.py Resolved

Comment thread astropy/utils/tests/test_decorators.py Resolved

nstarman marked this pull request as ready for review June 21, 2024 23:12

nstarman requested a review from a team as a code owner June 21, 2024 23:12

nstarman requested review from a team, mhvk and nstarman and removed request for a team and nstarman June 21, 2024 23:12

nstarman reviewed Jun 21, 2024

View reviewed changes

neutrinoceros force-pushed the cosmology/depr/15859/deprecate_redshift_as_keyword branch from d6efe8d to 254fa3b Compare June 22, 2024 07:13

nstarman approved these changes Jun 24, 2024

View reviewed changes

pllim reviewed Jun 24, 2024

View reviewed changes

pllim added the benchmark Run benchmarks for a PR label Jun 24, 2024

neutrinoceros force-pushed the cosmology/depr/15859/deprecate_redshift_as_keyword branch from 4c2a5cc to 7588291 Compare June 25, 2024 08:04

neutrinoceros force-pushed the cosmology/depr/15859/deprecate_redshift_as_keyword branch from 006a163 to bf51a29 Compare June 26, 2024 08:37

neutrinoceros mentioned this pull request Jun 26, 2024

Reminder: remove astropy.cosmology._utils.deprecated_keywords for astropy 8.0.0 #16614

Closed

pllim reviewed Jun 26, 2024

View reviewed changes

Comment thread astropy/utils/decorators.py Resolved

Comment thread astropy/utils/setup_package.py Resolved

nstarman added 2 commits June 26, 2024 17:46

redshift methods are positional-only

7cc0350

Signed-off-by: nstarman <[email protected]>

changelog

b402b07

neutrinoceros force-pushed the cosmology/depr/15859/deprecate_redshift_as_keyword branch from bf51a29 to 7d73b01 Compare June 26, 2024 15:46

DEPR: deprecate passing redshift (z) as a keyword argument

d2661dd

neutrinoceros force-pushed the cosmology/depr/15859/deprecate_redshift_as_keyword branch from 7d73b01 to d2661dd Compare June 26, 2024 15:47

pllim reviewed Jun 26, 2024

View reviewed changes

nstarman merged commit 00f2bba into astropy:main Jul 10, 2024

neutrinoceros deleted the cosmology/depr/15859/deprecate_redshift_as_keyword branch July 10, 2024 21:00

neutrinoceros mentioned this pull request Oct 10, 2024

feat: scale factor #17151

Merged

neutrinoceros mentioned this pull request Dec 2, 2024

API: mark most arguments from simple_norm as future keyword-only #17489

Merged

1 task

pllim mentioned this pull request Nov 17, 2025

Reminder: consider removing unused, internal C code #18951

Open

		@deprecated_keywords("z", since="7.0")
		def scale_factor(self, z):

		# F = TypeVar("F", bound=Callable[..., Any])
		F = Callable[..., Any]

	# F = TypeVar("F", bound=Callable[..., Any])
	F = Callable[..., Any]
	_F = TypeVar("_F", bound=Callable[..., Any])

Conversation

neutrinoceros commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Jun 20, 2024

Uh oh!

github-actions Bot commented Jun 20, 2024

Uh oh!

neutrinoceros commented Jun 20, 2024

Uh oh!

nstarman commented Jun 20, 2024

Uh oh!

neutrinoceros commented Jun 21, 2024

Uh oh!

neutrinoceros commented Jun 21, 2024

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

mhvk commented Jun 22, 2024

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

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 Jun 25, 2024 • 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!

mhvk commented Jun 25, 2024

Uh oh!

neutrinoceros commented Jun 25, 2024

Uh oh!

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

Uh oh!

Uh oh!

pllim commented Jun 25, 2024

Uh oh!

neutrinoceros commented Jun 26, 2024

Uh oh!

pllim commented Jun 26, 2024

Uh oh!

neutrinoceros commented Jun 26, 2024

Uh oh!

pllim commented Jun 26, 2024

Uh oh!

pllim commented Jun 26, 2024

Uh oh!

neutrinoceros commented Jun 26, 2024

Uh oh!

mhvk commented Jun 26, 2024

neutrinoceros commented Jun 20, 2024 •

edited

Loading

nstarman Jun 25, 2024 •

edited

Loading

nstarman commented Jun 25, 2024 •

edited

Loading

nstarman commented Jun 26, 2024 •

edited

Loading

neutrinoceros commented Jul 9, 2024 •

edited

Loading

nstarman commented Jul 10, 2024 •

edited

Loading