redshift methods are positional-only - #15859

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?
• Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
• 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.

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

I would like to point out that it's in fact possible for this process to go through a deprecation period. Here's my technical solution: https://gist.github.com/neutrinoceros/3f703d63761794b5e966330d51436b3c
I think matplotlib also has internal tooling to the same effect which might be more refined and battle tested.

Interesting! I like the idea. However, many of these functions need to be fast. What kind of overhead does this add?

I looked at the usage of passing z as a keyword arguments. See here. It seems to be used enough to merit a deprecation.

wow, I didn't know you could craft as fine a query on GitHub ! how did you do it ?

What kind of overhead does this add?

I guess it should be about the cost a couple additional function calls, but I never actually measured it. The one time this was used (to my knowledge) was for a gateway function in yt that 100% users use 100% of the time but which is not performance critical.

wow, I didn't know you could craft as fine a query on GitHub ! how did you do it ?

Laboriously 😆. In the search box. GH supports "advanced" syntax.

I guess it should be about the cost a couple additional function calls, but I never actually measured it. The one time this was used (to my knowledge) was for a gateway function in yt that 100% users use 100% of the time but which is not performance critical.

This could be rewritten in C and wrapped to Python. That would eliminate almost all performance overhead. E.g. something like https://github.com/ntessore/positional_defaults/blob/main/_positional_defaults.c, which rearranges positional arguments.

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

@nstarman do you need a hand to get this across the finish line ?

If you're up for it, that would be immensely appreciated! The issue is at a fair number of external libraries depend on the keyword form of the argument, meaning we would have to support a deprecation. The only way to do this is with a decorator that detects keyword access, raises a warning, and passes in the argument by position. This decorator would be slow unless it was written in c(ython). I just haven't found the time to write this decorator.

I can pick it up and give it a try after the Coordination Meeting, so say about a week from now.

@nstarman turns out I had some free time yesterday so I hacked together a proof of concept. Now I'm confident that this approach (using a C extension) works and is feasible. Would you invite me to collaborate on your astropy fork so I can push to this PR ? Just so you know, the soonest I'd be able to actually do it would be Monday, but there's no rush on my side :)

Amazing! That would be great.

@nstarman Would you prefer that I push directly here (and presumably take over your branch for stuff like rebasing) or that I open PRs on you fork targetting this branch instead ?
in any case, we should rebase this branch now to refresh CI and fix conflicts. I would also suggest removing the "Close?" label (which I don't have the power to do yet). Thank you

We should also apply the benchmark label, btw

edit: maybe not actually, there are very few benchmarks for cosmology and none of them seem to measure these functions specifically

Meanwhile, I'm putting together my own branch (based of this one) and doing some peformance measurements.

Here's a visual demo of my current results:
I measured function call overhead for 3 noop functions (accepting 0, 1 and 5 arguments respectively). Each graph shows results for functions called with 0, 1 and 3 keyword arguments respectively (each time, excluding functions that accept less).
Each call overhead is shown in 4 variants:

• no decorator (calling the pure Python function), this is by definition shown as "1" overhead
• @noop(): decorated with a no-op decorator in pure Python
• @defaults() : decorated with the @default decorator (implemented in C) from the positional_defaults package discussed above and which serves as a reference for a C based decorator
• @fpo(): decorated with my @future_positional_only decorator (implemented in C)

x2 overhead is annotated as "optimal" performance because, even wrapped in C, a wrapped function generates twice as many function calls, so I don't think we can hope for better than that.

I'm quite happy with the current performance: as explained, we're already at optimal perf in the case 0 keyword arguments are received, and we're still doing better than a pure Python decorator that does nothing in case we have 5 keywords to process, so we should minimally impact performance when users pass arguments positionally (which is exactly what we're asking them to do anyway).

@neutrinoceros this is great! Thanks for everything. I think it would be best if you cherry-picked my two commits onto a branch you own and open a new PR.

Here you go #16597 👀