[issue28754] Argument Clinic for bisect.bisect_left

Raymond Hettinger added the comment: FWIW, the itertools module has many functions that can have the ArgumentClinic applied without any fundamental problems (pretty much all of the functions will work except for repeat(), accumulate(), and islice()). --

[issue28754] Argument Clinic for bisect.bisect_left

Raymond Hettinger added the comment: After more thought, I'm going to close this one because the Argument Clinic and signature objects aren't yet a good fit for this API. The bisect module isn't broken (its API has been successfully living in the wild for a very long time). I do welcome

[issue28754] Argument Clinic for bisect.bisect_left

Raymond Hettinger added the comment: Wouldn't it be easily to let the OP apply AC to some other module. Right now, it isn't a very good fit and was this a tough one to start with. -- ___ Python tracker

[issue28754] Argument Clinic for bisect.bisect_left

Serhiy Storchaka added the comment: Now `Py_ssize_t(accept={int, NoneType})` can be used instead of a local converter. -- ___ Python tracker ___

[issue28754] Argument Clinic for bisect.bisect_left

Changes by Julien Palard : -- pull_requests: +143 ___ Python tracker ___ ___

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: As the pattern of this converter is not widely used, I'll let it in the code of _bisect for the moment, see: http://bugs.python.org/issue28933. -- ___ Python tracker

[issue28754] Argument Clinic for bisect.bisect_left

Larry Hastings added the comment: We don't *have* to use a converter function, no. But consider: someone somewhere will have to convert a PyObject * into a Py_ssize_t, also accepting None. Doing it in a converter function means we can easily share the code with bisect_right, which should

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: Hi Raymond, I don't like having the converters in the C implementation too, that's why I'm working on issue28933 to clean this. > letting the C function handle both -1 and None in the implementation rather > than in AC? It works, yes. But I prefer to clearly

[issue28754] Argument Clinic for bisect.bisect_left

Raymond Hettinger added the comment: Why does this need a converter function? I there is any reason this can't use "O" with "hi" defaulting to None (matching the pure python API) and letting the C function handle both -1 and None in the implementation rather than in AC? --

[issue28754] Argument Clinic for bisect.bisect_left

Larry Hastings added the comment: Okay, but do it with a converter function, not by changing the default behavior for Py_ssize_t. The vast majority of uses of Py_ssize_t should not accept None. -- ___ Python tracker

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: Hi Larry, In any cases it looks like supporting hi=-1 and hi=None is mandatory: hi=None is the current implementation in the bisect (Python) module. hi=-1 is the current implementation in the _bisect (C) module. Both are currently living together, the C

[issue28754] Argument Clinic for bisect.bisect_left

Larry Hastings added the comment: FWIW, pypy uses defaults of lo=0, hi=None. https://bitbucket.org/pypy/pypy/src/25da7bc9719457aee57547993833d5168ba6aded/lib-python/2.7/bisect.py?at=default=file-view-default -- ___ Python tracker

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: Maybe wait for issue28933. -- ___ Python tracker ___ ___ Python-bugs-list

[issue28754] Argument Clinic for bisect.bisect_left

Changes by Julien Palard : Added file: http://bugs.python.org/file45836/issue28754-9.diff ___ Python tracker ___

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: Added a test to ensure compatibility of both hi=None (introduced in original Python version) and hi=-1 (Introduced by the C version). Modified Python version to be compatible with the C-introduced hi=-1, so that the new test pass. --

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: I was able to simplify my patch a bit, I think I should also add a test to ensure we keep the hi=-1 and hi=None compatibility in the future. -- Added file: http://bugs.python.org/file45832/issue28754-8.diff ___

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: Hi Martin, Historically (lo=0, hi=None) was the implementation (in Python), until the C implementation, which used (lo=0, hi=-1). As my implementation allows for -1 and None, I don't think I'm breaking something. --

[issue28754] Argument Clinic for bisect.bisect_left

Martin Panter added the comment: Fair enough, I don’t really mind if it is (lo=0, hi=None). I think I have only used bisect with both defaults anyway. -- ___ Python tracker

[issue28754] Argument Clinic for bisect.bisect_left

Raymond Hettinger added the comment: > If adding proper support for hi=None, maybe lo=None should > also be supported. That would be gratuitous. Lo already has a reasonable, useful, and self-explanatory value. This would add more complexity to the signature while reducing clarity. I really

[issue28754] Argument Clinic for bisect.bisect_left

Martin Panter added the comment: If adding proper support for hi=None, maybe lo=None should also be supported. Also, I would think the main Doc/library/bisect.rst documentation needs updating, and a test and What’s New entry added. -- ___ Python

[issue28754] Argument Clinic for bisect.bisect_left

Stefan Krah added the comment: Signature and docstring can be done manually with very little effort. Currently METH_FASTCALL is AC only, but I hope that will change. -- ___ Python tracker

[issue28754] Argument Clinic for bisect.bisect_left

STINNER Victor added the comment: Stefan Krah added the comment: > Julien, the syntax converters look pretty clever. Do we need AC > everywhere though? Please see previous comments for advantages of AC (signature object, docstring, speed). -- ___

[issue28754] Argument Clinic for bisect.bisect_left

Stefan Krah added the comment: Julien, the syntax converters look pretty clever. Do we need AC everywhere though? I wonder (once again) if this is really more readable than the existing code. -- nosy: +skrah ___ Python tracker

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: I tried myself at Argument Clinic custom converters to create the "optional ssize_t converter" and it works, so as advised by Raymond, pydoc now shows None, and the C implementation now allows for both default values None and -1, as the custom converter

[issue28754] Argument Clinic for bisect.bisect_left

Martin Panter added the comment: Regarding the problem with the default value, can we use “unspecified”, as documented at , or is that an old relic that no longer exists (hinted at the end of Issue 20293)? Failing

[issue28754] Argument Clinic for bisect.bisect_left

STINNER Victor added the comment: Ah right, _bisect.bisect_right() support keywords. I expected that it doesn't support keywords yet ;-) -- ___ Python tracker

[issue28754] Argument Clinic for bisect.bisect_left

Serhiy Storchaka added the comment: > Sorry, I didn't follow the discussion, but why not using "/" magic separator > in Argument Clinic to mark arguments as optional but don't document their > default value? '/' marks positional-only arguments, not optional arguments. --

[issue28754] Argument Clinic for bisect.bisect_left

Serhiy Storchaka added the comment: Yet one option is to introduce the constant UNBOUND = -1 in the bisect module and use it as a default value. -- ___ Python tracker

[issue28754] Argument Clinic for bisect.bisect_left

STINNER Victor added the comment: Sorry, I didn't follow the discussion, but why not using "/" magic separator in Argument Clinic to mark arguments as optional but don't document their default value? -- ___ Python tracker

[issue28754] Argument Clinic for bisect.bisect_left

Raymond Hettinger added the comment: Sorry Julien, you don't to decide that long-standing APIs that have worked just fine for users are now a bad practice. Some of these APIs (range, dict.pop, type) we designed by Guido and work fine in practice. The expression, "don't have the tail wag the

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: Hi Raymond, About Argument Clinic # Just to clarify the situation, Argument Clinic allows for clear method signature, typically: `hi: Py_ssize_t(py_default="len(a)") = -1` gives the expected "hi=len(a)" signature successfully. The problem

[issue28754] Argument Clinic for bisect.bisect_left

Raymond Hettinger added the comment: After some further thought, I am open to changing the C API to accept either hi=None or hi=-1 to reconcile the implementation details without breaking any existing code that relies on either. That would let the argument clinic expose a default value of

[issue28754] Argument Clinic for bisect.bisect_left

Raymond Hettinger added the comment: > I would suggest to use sys.maxsize for default value in Argument Clinic No thanks. I don't want to subtly change the API for this module or even suggest to users that it might be a good idea to set hi > len. Further, we don't want to slow the pure

[issue28754] Argument Clinic for bisect.bisect_left

Serhiy Storchaka added the comment: I would suggest to use sys.maxsize for default value in Argument Clinic, but right now bisect functions don't support well hi > len(a). This needs adding an equivalent of hi = min(hi, len(a)) in C code. -1 could be explicitly supported for backward

[issue28754] Argument Clinic for bisect.bisect_left

Raymond Hettinger added the comment: [SS] > This is rather a random difference. Try to run the bench several times. I did run several times. The results were perfectly stable (+/- 1ns). It tells us that METH_FASTCALL is something we want to use as broadly as possible. [JP] > it yielded a

[issue28754] Argument Clinic for bisect.bisect_left

STINNER Victor added the comment: Argument Clinic adds a signature and a better docstring. So I like it! Speedup for tiny lists is just a nice side effect. -- ___ Python tracker

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: Hi Serhiy, hi: Py_ssize_t(py_default="len(a)") = -1 Won't works, as pydoc will use the inspect module (_signature_fromstr) to get the signature. _signature_fromstr expects a valid python signature, but `def foo(a, x, lo=0, high=len(a)): pass` is not valid

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: Hi Serhiy, > Julien, you can declare the hi parameter as >hi: Py_ssize_t(py_default="len(a)") = -1 Looks like a good idea, I was aware of its existance but did not took the time to read the doc about it, kind of learning step by stpe. But as you provided

[issue28754] Argument Clinic for bisect.bisect_left

Serhiy Storchaka added the comment: Julien, you can declare the hi parameter as hi: Py_ssize_t(py_default="len(a)") = -1 if you want the signature be matching the documentation. In general, it is better to use converter names rather than format codes. > Curiously, this patch gives about

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: Hi Raymond, > Curiously, this patch gives about a 10% to 15% speedup. Any sense of how > that improvement arises? That's because Argument Clinic is generating methoddef with METH_FASTCALL: $ grep FASTCALL Modules/clinic/_bisectmodule.c.h

[issue28754] Argument Clinic for bisect.bisect_left

Raymond Hettinger added the comment: Curiously, this patch gives about a 10% to 15% speedup. Any sense of how that improvement arises? $ py3.7 -m timeit -s 'from bisect import bisect' -s 'a=list(range(100))' 'bisect(a, 10)' 229 nsec # Clang baseline 202 nsec # Clang with patch 189

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: Should we also update howto/clinic, bullet "11.", to be explicit about not adding per-parameter in the same patch? Like a: > If you're porting existing function to Argument clinic, skip this step to > simplify code review. ? --

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: Here is the new patch, I ran a diff between "./python -m pydoc _bisect" before and after my patch, here it is: 13,15c13 < bisect_left(...) < bisect_left(a, x[, lo[, hi]]) -> index < --- > bisect_left(a, x, lo=0, hi=-1) 25,27c23 <

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: Quoting @martin > * bisect_right(a, x[, lo[, hi]]) -> index > This signature is removed. I think removing it is reasonable, because pydoc > can extract the proper signature from the Arg Clinic metadata. In fact, http://docs.python.org/howto/clinic.html, bullet

[issue28754] Argument Clinic for bisect.bisect_left

Martin Panter added the comment: Taking the first function, bisect_right(), as an example, I see these differences: * bisect_right(a, x[, lo[, hi]]) -> index This signature is removed. I think removing it is reasonable, because pydoc can extract the proper signature from the Arg Clinic

[issue28754] Argument Clinic for bisect.bisect_left

STINNER Victor added the comment: Martin: "Victor, what changes to the doc strings are you talking about?" See attached check_bisect_doc.py script. Before: --- === bisect_right === bisect_right(a, x[, lo[, hi]]) -> index Return the index where to insert item x in list a, assuming a is sorted.

[issue28754] Argument Clinic for bisect.bisect_left

Martin Panter added the comment: Victor, what changes to the doc strings are you talking about? Adding descriptions of the parameters, maybe? Other than that they look very similar to me. I haven’t looked closely, but the Python doc strings have been updated at least once more than the C doc

[issue28754] Argument Clinic for bisect.bisect_left

STINNER Victor added the comment: issue28754-4.diff does 3 things: * convert C code to Argument Clinic * change docstrings of the C code * change unit tests issue28754-5.diff doesn't touch tests, but still does 2 things. Sadly, I dislike the changes on docstrings because it makes the C

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: New simplier patch thanks to https://bugs.python.org/issue28792. Also corrected docstrings. Also ran a micro-benchmark of `bisect.bisect(foo, "c")` with `foo = list("abcdef")`: Median +- std dev: [before] 434 ns +- 17 ns -> [after] 369 ns +- 22 ns: 1.18x

[issue28754] Argument Clinic for bisect.bisect_left

STINNER Victor added the comment: Hum, I dislike your changes on test_bisect.py: I don't see why Argument Clinic would have to modify tests!? I created issue #28792: "bisect: implement aliases in Python, remove C aliases". -- nosy: +haypo ___

[issue28754] Argument Clinic for bisect.bisect_left

Changes by Julien Palard : Added file: http://bugs.python.org/file45626/issue28754-4.diff ___ Python tracker ___

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: The whole diff is reviewable in `issue28754-3.diff`. -- ___ Python tracker ___

[issue28754] Argument Clinic for bisect.bisect_left

Changes by Julien Palard : Added file: http://bugs.python.org/file45572/issue28754-3.diff ___ Python tracker ___

[issue28754] Argument Clinic for bisect.bisect_left

Changes by Julien Palard : Added file: http://bugs.python.org/file45571/insort-left.diff ___ Python tracker ___

[issue28754] Argument Clinic for bisect.bisect_left

Changes by Julien Palard : Added file: http://bugs.python.org/file45568/bisect_left.diff ___ Python tracker ___

[issue28754] Argument Clinic for bisect.bisect_left

Changes by Julien Palard : Added file: http://bugs.python.org/file45570/insort.diff ___ Python tracker ___

[issue28754] Argument Clinic for bisect.bisect_left

Changes by Julien Palard : Added file: http://bugs.python.org/file45569/bisect.diff ___ Python tracker ___

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: Here it is for the whole bisect module. I separated my work in commits, but I'm not sure how rietveld will eat that as they'll have unknown references, so I'll probably also upload a single patch with a known reference. --

[issue28754] Argument Clinic for bisect.bisect_left

Julien Palard added the comment: I searched an occurrence of what I'm describing which is already using clinic and there is, at least, one in Modules/binascii.c line 1090: TL;DR: The idea is to use the `modulename.fnname [as c_basename] = modulename.existing_fn_name` clinic syntax, drop a

[issue28754] Argument Clinic for bisect.bisect_left

Changes by Serhiy Storchaka : -- nosy: +rhettinger versions: +Python 3.7 ___ Python tracker ___

[issue28754] Argument Clinic for bisect.bisect_left

Larry Hastings added the comment: Oh, and, if the code literally asserts they're the same function, that's just a sanity check based on the implementation. You could preserve that if you care to, or you could just write a new function and remove the assertion. Do what you think is best,

[issue28754] Argument Clinic for bisect.bisect_left

Larry Hastings added the comment: There's special syntax to handle aliases. From comments in clinic.py: # alternatively: # modulename.fnname [as c_basename] = modulename.existing_fn_name # clones the parameters and return converter from that # function. you

[issue28754] Argument Clinic for bisect.bisect_left

Changes by Julien Palard : Added file: http://bugs.python.org/file45563/issue28754-2.diff ___ Python tracker ___

[issue28754] Argument Clinic for bisect.bisect_left

New submission from Julien Palard: Today I read https://docs.python.org/3.6/howto/clinic.html so I tried one: bisect.bisect_left. I was unable to do `bisect_right`, as it's an "alias" for `bisect`, and there's a unit-test checking `self.assertEqual(self.module.bisect,

[issue28754] Argument Clinic for bisect.bisect_left

Changes by Julien Palard : -- keywords: +patch Added file: http://bugs.python.org/file45562/issue28754.diff ___ Python tracker