sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.31k stars 449 forks source link

Merge sage_brial into sagelib #30332

Closed mkoeppe closed 4 years ago

mkoeppe commented 4 years ago

As discussed in #29369 and #28918.

CC: @kiwifb @orlitzky @tscrim

Component: packages: standard

Author: François Bissey

Branch/Commit: 91fe5e1

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/30332

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 64c57d2 to 878d941

kiwifb commented 4 years ago
comment:34

Last commit to burn the bridges :)

mkoeppe commented 4 years ago
comment:35

BooleanPolynomialRing_constructor vs. BooleanPolynomialRing looks a bit confusing

kiwifb commented 4 years ago
comment:36

Replying to @mkoeppe:

BooleanPolynomialRing_constructor vs. BooleanPolynomialRing looks 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.

mkoeppe commented 4 years ago
comment:37

Sure, just trying to figure out what the high level interface is intended to be.

mkoeppe commented 4 years ago
comment:38

build/pkgs/sagelib/dependencies still has sage_brial

kiwifb commented 4 years ago
comment:39

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

02a5af1build/pkgs/sagelib/dependencies: Remove sage_brial
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 878d941 to 02a5af1

mkoeppe commented 4 years ago

Author: François Bissey

tscrim commented 4 years ago
comment:43

Replying to @kiwifb:

Replying to @mkoeppe:

BooleanPolynomialRing_constructor vs. BooleanPolynomialRing looks 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.

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.

kiwifb commented 4 years ago
comment:44

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 02a5af1 to 0c5fe64

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

754dea9Properly format tests in gbcore
0c5fe64Merge branch 'public/merge_sage_brial' of trac.sagemath.org:sage into brial-merging
tscrim commented 4 years ago
comment:46

I think that is a better plan. I am too stretched for time right now to work on this.

mkoeppe commented 4 years ago

Reviewer: Matthias Koeppe

mkoeppe commented 4 years ago
comment:47

I agree with this plan - it will also be a good basis for modularization work in 9.3

kiwifb commented 4 years ago
comment:48

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

91fe5e1Fix doctest and docstring formatting
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 0c5fe64 to 91fe5e1

kiwifb commented 4 years ago
comment:50

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.

vbraun commented 4 years ago

Changed branch from public/merge_sage_brial to 91fe5e1