Opened 2 years ago
Closed 2 years ago
#30332 closed enhancement (fixed)
Merge sage_brial into sagelib
| Reported by: | Matthias Köppe | Owned by: | |
|---|---|---|---|
| Priority: | major | Milestone: | sage-9.2 |
| Component: | packages: standard | Keywords: | |
| Cc: | François Bissey, Michael Orlitzky, Travis Scrimshaw | Merged in: | |
| Authors: | François Bissey | Reviewers: | Matthias Koeppe |
| Report Upstream: | N/A | Work issues: | |
| Branch: | 91fe5e1 (Commits, GitHub, GitLab) | Commit: | 91fe5e1c34a3d0b0e4a2960119aa313ad51e687e |
| Dependencies: | Stopgaps: |
Status badges
Change History (52)
comment:1 Changed 2 years ago by
comment:2 Changed 2 years ago by
So there's a Cython interface sage.libs.polybori, then a Python library brial, and also sage.rings.polynomial.pbori? What is depending on what?
comment:3 Changed 2 years ago by
I thought that sage-brial depends on sage.libs.polybori but that may not be the case. It imports sage.all and specifically sage.rings.polynomial.pbori and that's the only sage specific bits it imports.
sage.rings.polynomial.pbori itself relies on sage.libs.polybori (through sage.libs.polybori.decl) and both need the C++ brial library.
Does that answer your question?
comment:4 Changed 2 years ago by
Thanks. I agree that sage.rings.polynomial seems to be a good place for the Python code from sage-brial. Perhaps it would be best to create a package sage.rings.polynomial.brial and to move the current pbori.pyx inside it (leaving deprecated reimport behind). There are only a few places in Sage where things are imported from sage.rings.polynomial.pbori and they could be easily update. Or, if you don't mind keep the old name pbori, the same could be done with sage.rings.polynomial.pbori instead of ...brial.
One thing to keep in mind is the context of the planned modularization of sagelib.
In Sage 9.3 I would hope to be able to create a separate distribution (distutils package) sage-brial, which would then package sage.rings.polynomial.{brial,pbori}, sage.libs.polybori, sage.libs.fes, sage.crypto.boolean_function -- everything that has a compile-time dependency on libbrial. This will make it possible to separately compile sagelib on systems without brial.
comment:5 follow-up: 6 Changed 2 years ago by
On the legal side, the code is GPL2.0 or later. Although I will need to fix the license on github. It seems I changed it from 2+ to 2 carelessly back in 2017 (probably by copying a template). I obviously had no rights to do that.
The only thing I may need guidance with is the deprecated re-import. Otherwise moving stuff, including pbori.{pyx,pxd}, in sage.rings.polynomial.pbori seems to be a good idea and hopefully will help with modularization.
comment:6 follow-up: 7 Changed 2 years ago by
Replying to fbissey:
On the legal side, the code is GPL2.0 or later. Although I will need to fix the license on github. It seems I changed it from 2+ to 2 carelessly back in 2017 (probably by copying a template). I obviously had no rights to do that.
I think GitHub takes the position that there is no such thing as "GPL 2 only": That the license includes "... or any later version" no matter whether you write these words in the files or not.
comment:7 Changed 2 years ago by
Replying to mkoeppe:
Replying to fbissey:
On the legal side, the code is GPL2.0 or later. Although I will need to fix the license on github. It seems I changed it from 2+ to 2 carelessly back in 2017 (probably by copying a template). I obviously had no rights to do that.
I think GitHub takes the position that there is no such thing as "GPL 2 only": That the license includes "... or any later version" no matter whether you write these words in the files or not.
Very nice of them. But they are not lawyers and I think the "LICENSE" file should be explicit for people who look for these subtle clarification (distros). The GPL FAQ https://www.gnu.org/licenses/old-licenses/gpl-2.0-faq.html#VersionTwoOrLater seem to imply that you should explicitly state it.
comment:8 Changed 2 years ago by
Tests included inside sage-brial will have to be moved to sage doctesting. More generally it will have to be documented properly as much as possible. This will be so much fun.
It looks like that will take much more than just drag and drop in place with a few cosmetic changes.
comment:9 Changed 2 years ago by
I suppose you could prepare the change to using the sage doctester first, before merging into Sage.
comment:10 follow-up: 11 Changed 2 years ago by
Given that sage_brial has no Cython bits, I think it could in fact be removed from the dependencies of sagelib, as it won't really be needed while installing sagelib, only later for docbuild and doctest.
comment:11 Changed 2 years ago by
Replying to mkoeppe:
Given that sage_brial has no Cython bits, I think it could in fact be removed from the dependencies of sagelib, as it won't really be needed while installing sagelib, only later for docbuild and doctest.
Technically correct.
comment:12 Changed 2 years ago by
| Branch: | → public/merge_sage_brial |
|---|---|
| Commit: | → 884ca6663e3e18dd284774b1a3f51b30fdb50882 |
This is not ready for review. I am just pushing my work in progress so it is not just on my workstation, it is visible and open to others contribution.
There is a lot to do:
- update paths
- convert docstrings and tests to sage framework
This may take some time and I doubt it will be ready in time for 9.2 but we can only try.
New commits:
| 884ca66 | WIP: move sage-brial files and pbori.{pxd,pyx} in sage/rings/polynomial/pbori/
|
comment:13 Changed 2 years ago by
| Commit: | 884ca6663e3e18dd284774b1a3f51b30fdb50882 → e7653a3417bf7657cfdb65a4685b74284f644b6d |
|---|
comment:14 Changed 2 years ago by
| Commit: | e7653a3417bf7657cfdb65a4685b74284f644b6d → 2226097a9bf79f4b9c9675fc9fe73ffd043819e6 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 2226097 | src/sage/rings/polynomial/pbori/__init__.py: Draft of lazy_import call
|
comment:15 Changed 2 years ago by
| Commit: | 2226097a9bf79f4b9c9675fc9fe73ffd043819e6 → d0bc7863a60366121688bb09f64af3618d3e1edc |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| d0bc786 | transform python test into sage tests - step one.
|
comment:16 Changed 2 years ago by
I am currently converting docstrings :( I am also doing some selective code deletion as I come along across it. There is a module for memory usage, it is imported once but never used and even less tested. So, I am removing it. It will be worth another look after that first pass.
comment:17 Changed 2 years ago by
| Commit: | d0bc7863a60366121688bb09f64af3618d3e1edc → bdf1fecd4a5532f09abf6cb204dfeb705dfd253e |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| ae30cf3 | Basic conversion to sage doctrings/doctesting. Remove original copyright/license functions.
|
| 4c0d80f | Some basic docstring in general_boolean_polynomial.py - remove memusage.py and its only import.
|
| bdf1fec | First pass in converting to docstring and doctests. It is quite likely a lot of doctests are broken at this stage.
|
comment:18 Changed 2 years ago by
| Commit: | bdf1fecd4a5532f09abf6cb204dfeb705dfd253e → 2e94a7725ea5018c6cfd8cd291df80bcf0c7a846 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 2e94a77 | Merge branch 'develop' into brial-merging
|
comment:19 Changed 2 years ago by
I suspect the building of the html doc will break at this stage. If it doesn't the output may very well be useless. Once I figure that out, I'll do the doctesting to see what's broken. I am expecting a lot of breaking in the newly imported tests.
comment:20 Changed 2 years ago by
| Commit: | 2e94a7725ea5018c6cfd8cd291df80bcf0c7a846 → 4e3ed8850f451ee6f65526434397cc880cd38f0c |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 4e3ed88 | one too many pbori.
|
comment:21 Changed 2 years ago by
| Commit: | 4e3ed8850f451ee6f65526434397cc880cd38f0c → 563e959d6dcf27f9352f5ae3590b2b35148fe2bb |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 563e959 | add miising import of lazy_import
|
comment:22 Changed 2 years ago by
| Commit: | 563e959d6dcf27f9352f5ae3590b2b35148fe2bb → 408824d44f039c51ba2ab8c9552bf2a0f8cfedcc |
|---|
comment:23 Changed 2 years ago by
| Commit: | 408824d44f039c51ba2ab8c9552bf2a0f8cfedcc → 443a3a8d5d3ba3b05db9312f33788d845836808d |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 443a3a8 | Remove addition.py - nothing from this file is used and tests failures point to the file being obsolote.
|
comment:24 Changed 2 years ago by
| Commit: | 443a3a8d5d3ba3b05db9312f33788d845836808d → 167ef09e38e16eee05a7c278c47558772666e6e8 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 167ef09 | migrate xrange (unsupported in python3) to range
|
comment:25 Changed 2 years ago by
At this point documentation builds and is identical to the old one. doctesting gets a lot of failures. There is still quite a bit of cleaning to be done, including removing files that should have been trashed long ago.
comment:26 Changed 2 years ago by
More joy! I found a subtle coding error while fixing the test suite. I guess it won't have an impact on the rest of sage because that code path just error-ed out when taken. So it is probably a dead code path anyway.
comment:27 Changed 2 years ago by
| Commit: | 167ef09e38e16eee05a7c278c47558772666e6e8 → e25dd810b818d1543b66313d608ca33d93341b76 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| afdbb9d | fix doctests in blocks.py
|
| ec4cddb | remove check_claims.py which appears to be a standalone script.
|
| d53714f | remove another script
|
| 72f8a6a | Fix doctest and code in cnf.py
|
| e25dd81 | Fix pbori related doctests and code in multi_polynomial_sequence
|
comment:28 Changed 2 years ago by
| Commit: | e25dd810b818d1543b66313d608ca33d93341b76 → a0ccf77b2d4af9e5a6fde8c01c4f0d1fdfff2bee |
|---|
comment:29 Changed 2 years ago by
| Commit: | a0ccf77b2d4af9e5a6fde8c01c4f0d1fdfff2bee → 20d1f9e9fb6e4681fe45c41daf25b915788f8295 |
|---|
comment:30 Changed 2 years ago by
| Commit: | 20d1f9e9fb6e4681fe45c41daf25b915788f8295 → bc1f9e24bdd9125fc48ed8de59232eb09e950e80 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| bc1f9e2 | fix more doctests
|
comment:31 Changed 2 years ago by
| Commit: | bc1f9e24bdd9125fc48ed8de59232eb09e950e80 → 64c57d2bc82f3920d8c91ca06669d5e4bb14df48 |
|---|
comment:32 Changed 2 years ago by
| Status: | new → needs_review |
|---|
This is ready for a first review. I am not expecting it to make it as is. But other people should comment now.
comment:33 Changed 2 years ago by
| Commit: | 64c57d2bc82f3920d8c91ca06669d5e4bb14df48 → 878d941065c696fc38d9ec59d917a0ca1e3375da |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 878d941 | Remove the sage_brial package since its meaningful content is merged.
|
comment:35 follow-up: 36 Changed 2 years ago by
BooleanPolynomialRing_constructor vs. BooleanPolynomialRing looks a bit confusing
comment:36 follow-up: 43 Changed 2 years ago by
Replying to mkoeppe:
BooleanPolynomialRing_constructorvs.BooleanPolynomialRinglooks a bit confusing
Not my code, I am only the messenger. I certainly agree that this largely decade old code would need a good fleeing - apart from the trimming I already did on import. I assumed we could do it after merging it.
comment:37 Changed 2 years ago by
Sure, just trying to figure out what the high level interface is intended to be.
comment:39 Changed 2 years ago by
Right. I remember thinking about that last night. Forgot to remove it. Will do in a few hours if no one beats me to it.
comment:40 Changed 2 years ago by
| Commit: | 878d941065c696fc38d9ec59d917a0ca1e3375da → 02a5af16c449d4839658aa5d7ac352dc2227f7d2 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 02a5af1 | build/pkgs/sagelib/dependencies: Remove sage_brial
|
comment:41 Changed 2 years ago by
| Authors: | → François Bissey |
|---|
comment:42 Changed 2 years ago by
| Cc: | Travis Scrimshaw added |
|---|
comment:43 Changed 2 years ago by
Replying to fbissey:
Replying to mkoeppe:
BooleanPolynomialRing_constructorvs.BooleanPolynomialRinglooks a bit confusingNot my code, I am only the messenger. I certainly agree that this largely decade old code would need a good fleeing - apart from the trimming I already did on import. I assumed we could do it after merging it.
Indeed, this is a known problem that needs some attention: #21892, #24748, possibly #27019, #23310, #23312, possibly #23311, #15223.
My idea is to scrap BooleanPolynomialRing_constructor and just have BooleanPolynomialRing be a UniqueRepresentation object, possibly just being a subclass of the monoid algebra parent.
comment:44 Changed 2 years ago by
My idea was to merge this in 9.2 and work on improving things in 9.3. However, if you know exactly what to do, we can work on this now.
comment:45 Changed 2 years ago by
| Commit: | 02a5af16c449d4839658aa5d7ac352dc2227f7d2 → 0c5fe64317e93d2bf9f58d1418f5a73ecbb4968e |
|---|
comment:46 Changed 2 years ago by
I think that is a better plan. I am too stretched for time right now to work on this.
comment:47 Changed 2 years ago by
| Reviewers: | → Matthias Koeppe |
|---|---|
| Status: | needs_review → positive_review |
I agree with this plan - it will also be a good basis for modularization work in 9.3
comment:48 Changed 2 years ago by
I am in the process of looking at adding some of the documentation from sage-brial to the sage documentation. At least to see how it looks like.
If you think that's not worth adding now, it is fine.
comment:49 Changed 2 years ago by
| Commit: | 0c5fe64317e93d2bf9f58d1418f5a73ecbb4968e → 91fe5e1c34a3d0b0e4a2960119aa313ad51e687e |
|---|---|
| Status: | positive_review → needs_review |
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
| 91fe5e1 | Fix doctest and docstring formatting
|
comment:50 Changed 2 years ago by
Just a touch up. One doctest was commented out so I decided to enable it. Building documentation revealed formatting issues in docstrings. All in the same file.
I built the doc with some added files from brial and we definitely should keep that for later work. Individual files need sensible title blocks for a start.
comment:51 Changed 2 years ago by
| Status: | needs_review → positive_review |
|---|
comment:52 Changed 2 years ago by
| Branch: | public/merge_sage_brial → 91fe5e1c34a3d0b0e4a2960119aa313ad51e687e |
|---|---|
| Resolution: | → fixed |
| Status: | positive_review → closed |
I propose to copy the current sage-brial code under
sage/rings/polynomial. This is based on the fact all (but one) imports of brial happen there.but other suggestions based on best practices will be given the priority.