DOC: PRIMA licence and reference fix #22738
Conversation
Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>
|
Maybe we should also put something in https://github.com/scipy/scipy/blob/main/LICENSES_bundled.txt ? I have to admit I didn't know about those 3 other files! |
|
@j-bowhay that's a better place to make the addition. |
|
Argh, the license issue also totally slipped my mind. Thanks for looking into it @andyfaff . |
|
Hello Andrew @andyfaff and others, Thank you very much for taking care of this. It is highly appreciated. I would like to suggest some changes. See andyfaff#80 Note that the PRIMA version of COBYLA is significantly different from Powell's original algorithm and code. An example is I also suggest removing two references that did not discuss COBYLA (Powell 1998 and Powell 2007) --- since COBYLA itself is difficult enough to understand, let's not confuse users with papers that are essentially irrelevant. I suggest (and humbly request) that the scipy documentations make it as clear as possible that the new version of COBYLA comes from the PRIMA package of Zaikun Zhang, so that people know who (namely, me) to blame if something goes wrong due to my changes to the algorithm. Forgive me for repeating the following:
Thank you very much. Zaikun |
|
A question, probably can be answered by @nbelakovski : does the interface of COBYLA in SciPy use I did notice https://github.com/zaikunzhang/scipy_prima/blob/prima/scipy/optimize/_cobyla_py.py#L173 It confuses me a bit. It seems that we receive Thank you. |
There was a problem hiding this comment.
Thanks for taking care of this @andyfaff.
Can I suggest also adding scipy/_lib/pyprima to https://scipy.github.io/devdocs/dev/core-dev/index.html#vendored-code?
My understanding of this is that we actually have two interfaces to COBYLA. We have
Doing a spot check using GitHub code search, the two ways of calling COBYLA seem roughly equally popular. |
|
I note that there is already a separate pyPRIMA package on PyPI. We should keep this in mind. |
It lists Zaikun as the maintainer and points to PDFO so no conflict there. @zaikunzhang, @nickodell's answer above to the question about tol vs rhoend is correct. |
Co-authored-by: nbelakovski <nbelakovski@users.noreply.github.com>
It is a placeholder by me, which will be used for PRIMA. |
|
What we have looks appropriate to me now. Authorship of PRIMA is referenced and the license is pointed to. |
| @@ -20,10 +20,12 @@ Currently, this includes the following parts of the codebase: | |||
| - QHull_, at ``scipy/spatial/qhull_src`` | |||
| - trlib_, at ``scipy/optimize/_trlib`` | |||
| - UNU.RAN_, at ``scipy/stats/_unuran`` | |||
| - PRIMA_, at ``scipy/_lib/pyprima`` | |||
There was a problem hiding this comment.
On second thought, this is unnecessary as we are actually vendoring this as a git submodule. Not a big deal though
There was a problem hiding this comment.
Indeed. I didn't realize we had changed plans; I thought we wouldn't add a submodule (no bandwidth to follow along unfortunately).
There was a problem hiding this comment.
my understanding was that we converged on:
- not adding a submodule to Nikolai's repo, instead 'mirroring' under the SciPy org (for security reasons)
- not adding a 'full' submodule, but rather using a script to trim off the parts we don't need to use (like the Fortran PRIMA implementation) while vendoring a specific commit hash
That script is https://github.com/scipy/pyprima/blob/main/vendor_pyprima.sh. I suppose we could just maintain the script and the vendored source in the main scipy repo, but I think the separation is nice.
There was a problem hiding this comment.
I suppose we could just maintain the script and the vendored source in the main scipy repo, but I think the separation is nice.
That's what I had thought we settled on indeed. The separation now just gives us the overhead of a git submodule without the ability to pull in or work on upstream commits without much of an upside, right?
Anyway, I'm not suggesting to change it. We have enough submodules, and this probably won't be updated too often.
There was a problem hiding this comment.
Agreed. I think the only upside of the obvious separation is making it clear that contributors shouldn't (/can't) modify the source in this repo.
But that could equally be achieved (at some point in the future) by a refactor of _lib which cleanly separates internal from external code (scikit-learn separates sklearn.utils and sklearn.externals for example).
This PR appends the PRIMA licence to the other bundled licences we use. In addition it adds references to PRIMA. Are you happy with this approach @zaikunzhang and @nbelakovski?
@j-bowhay