GalSim-developers / GalSim

The modular galaxy image simulation toolkit. Documentation:
http://galsim-developers.github.io/GalSim/
Other
227 stars 107 forks source link

Make most keyword parameters invalid as positional arguments #1130

Open rmjarvis opened 3 years ago

rmjarvis commented 3 years ago

Now that we don't have to support Python 2.7 anymore, we can start using the * in function signatures to indicate keyword-only arguments. IMO, this tends to make user code better, forcing them to self-document what things mean by naming parameters, rather than let them be cryptically just False or 3.5, etc.

In general, I think we should make almost everything keyword-only. The main exceptions are for the first argument of a function, when the meaning of that argument is clear from the name of the function. e.g. image.addNoise(noise) or obj.setFlux(flux). Occasionally there are cases in GalSim code where both of the first two arguments are fairly obvious from context. e.g. obj.shift(x,y).

The other main exception will be for our "leading underscore" functions. I don't know what the latest performance difference is between args and kwargs, but it at least used to be the case that kwarg parsing was slower (cf. https://bugs.python.org/issue27574). This makes some sense, since it requires some string parsing rather than just counting items in a list. I don't know how much of a difference it makes in practice, but since our leading underscore functions are designed to whittle away as much overhead as possible for performance, let's not add any back by making them use kwargs.

Beyond these kinds of cases, I think we should usually favor forcing the user to explicitly name any parameters they are using. But there may be some other exceptions here and there where it might make sense to allow a couple parameters to remain positional. I suspect we can probably use our own code (tests, examples, and internal calls) as a guide for when to allow positional arguments. If we found it clear enough to write something without a keyword, we might probably want to let users do so too.

This is technically an API change, but I have a decorator we can use during the 2.x series to make it warn for violations rather than raise TypeErrors, which I wrote for the corresponding effort for TreeCorr. cf. https://github.com/rmjarvis/TreeCorr/pull/129/files#diff-56ac2f79094c0869fb8bbe6744b6cbcd7e270e9f6cac90b9373993ec2c14524fR927