Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compute the characteristic varieties of a finitely presented group #36128

Merged
merged 57 commits into from
Sep 24, 2023

Conversation

enriqueartal
Copy link
Contributor

Let $G$ be a finitely presented group. For a field $\mathbb{k}$, let $\Lambda:=\mathbb{k}[G]$ be its group algebra and let $\mathbb{T}$ be its maximal spec, which is a torus. An element $\xi\in\mathbb{T}$ determines a local system of coefficients and the characteristic varieties of $G$ are defined as
$$V_k(G):=\{\xi\in\mathbb{T}\mid \dim H^1(G;\xi)\geq k\}.$$
Except for the neutral element these are the reduced varieties of the Fitting ideals of the Alexander matrix of the presentation. The function char_var gives systems of generators of these ideals. A groebner option is given to give Groebner basis; it is set as an option since the computations may be quite long.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

src/sage/groups/finitely_presented.py Outdated Show resolved Hide resolved
src/sage/groups/finitely_presented.py Outdated Show resolved Hide resolved
src/sage/groups/finitely_presented.py Outdated Show resolved Hide resolved
src/sage/groups/finitely_presented.py Outdated Show resolved Hide resolved
src/sage/groups/finitely_presented.py Outdated Show resolved Hide resolved
if not groebner or not R.base_ring().is_field():
return res
if R.ngens() == 1:
res0 = [[S(_) for _ in J.gens()] for J in res]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
res0 = [[S(_) for _ in J.gens()] for J in res]
res0 = [[S(g) for g in J.gens()] for J in res]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it also.

LJ = J.minimal_associated_primes()
fct = [id.groebner_basis() for id in LJ]
fct = [a for a in fct if S.gen() not in a]
if fct != [(S.one(),)]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mirco-optimization: pull out the [(S.one(),)] into a variable to avoid reconstructing the list and tuple every loop.

src/sage/rings/polynomial/laurent_polynomial_mpair.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/laurent_polynomial_mpair.pyx Outdated Show resolved Hide resolved
src/sage/groups/finitely_presented.py Outdated Show resolved Hide resolved
@tscrim
Copy link
Collaborator

tscrim commented Sep 18, 2023

@enriqueartal Something is horribly broken with the bot. Please don't try anything further. I have already reported it.

@enriqueartal
Copy link
Contributor Author

I realized that monomial_reduction gave a polynomial and an integer in the univariate case and a polynomial and a monomial in the multivariate case. Now, it is always the second case. Sorry for the mistake.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! How did you come across this? Is it simple to add a doctest about it somewhere? (By this, I mean not in the method itself, but instead in, say, called via the char variety method.)

I have one small way to improve the efficiency of the method too.

Comment on lines 1591 to 1592
u, n = self.polynomial_construction()
return (u, self.parent().gen(0) ** n)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
u, n = self.polynomial_construction()
return (u, self.parent().gen(0) ** n)
return (self.__u, self._parent.gen(0) ** self.__n)

Since you're in the class itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip, changed. Actually there was a doctest for the univariate case; since the code only uses the polynomial output of monomial_reduction the difference between the output of the methods in the uni- and multi-variate case didn't affect. Anyway, I found it strange.
I came into it because I was thinking about some improvements for Laurent polynomials (outside this PR), in order to apply later PR #36299. Actually there are quite a lot of things to do; for example, univariate Laurent polynomial rings have not the polynomial_ring method (as multivariate have), or they do not know to be PID when the base ring is a field.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Yes, there are lots of ways Laurent polynomials can be improved (and likely other methods that are not in sync between the uni- and multi-variate implementations).

Then back to positive review. Feel free to cc me on any subsequent tickets, I will try to review them when I have a chance.

@github-actions
Copy link

Documentation preview for this PR (built with commit 7a7c564; changes) is ready! 🎉

@vbraun vbraun merged commit 2cf443a into sagemath:develop Sep 24, 2023
18 of 19 checks passed
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 28, 2023
…tion

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

Unfortunately the fix in sagemath#36213 has
not been completed properly. There is still a `return` from the
`get_review_decision` method which is not `None` where it should be.

This is fixed here. Sorry for this unnecessary trouble and my lack of
attention!

In a next step this PR fixes a bug which has been noticed by an
accidentally activation of the `unlabeled` trigger (see
sagemath#36292 (comment)).
Simultanously the `labeled`-trigger has been disabled. This is an
unrealistic setting which was not covered by the code.

Explicitely, a state label could not be removed even though other state
labels where set (examples  sagemath#36128
and sagemath#36020). This PR leads to the
following changes of behavior:

1. The reaction on the `unlabeled`-event is reduced to the case of the
last label
2. The `unlabeled`-event will not lead to a rejection of the removal any
more
3. Instead of a warning comment a hint comment is posted.

We found a way to test the bot a little more realistically (see
soehms#11). Additional problems emerged and
were also resolved. This led to the following changes in the workflow:


4. A bug in `actor_valid` is fixed (see
soehms#10 (comment))
5. Preparing to use our own bot user (e.g.
[sagemathadmins](https://github.com/sagemathadmins)) (see
soehms#11 (comment))
6. Waiver of observing the `reviewDecision` feature provided by GitHub
(see 2. in
soehms#11 (comment))
7. Allow the user to revert his decision of label selection (see
soehms#11 (comment))
8. Don't reject label addition any more, except in the case where the
author tries to set `s: positive review` to his own PR which has no
reviews from others (see
soehms#11 (comment))
9. Dismiss stale reviews of the bot after a push to the branch and on
submission of a new review which is more than just a comment (see
soehms#11 (comment)).

Despite the fact that the testing was now more realistic, it is still
not guaranteed that the bot's behavior in `sagemath/sage` will be
completely covered by our testing.

The changes in this PR cannot be activated immediately after merging
into the develop branch due to a bug in the GitHub web interface
observed during testing in soehms#11 (see
soehms#11 (comment) and
soehms#11 (comment)). The
problem is that the panel at the top right of the webpage that contains
the labels does not update immediately after the bot changes the labels.
Since this might cause confusion, I'll create a bug report about it. The
going-live of the sync bot will be stalled at least until we received an
answer.

This PR changes the following flowcharts from
sagemath#35172 :

##### What happens when adding `s: needs review` to a PR?

```mermaid
---
title: add the needs review label
---
flowchart LR

%% vertices

    trigger([label\n 's: needs review'\n added])
    mark_as_ready([mark as\n ready for\n review])
    remove_other_labels([remove other\n state labels])
    warning_about_label_addition([warning\n about label\n addition])
    needs_review_valid[[needs review\n valid?]]
    is_draft{is PR\n a draft?}

%% edges

    trigger --> needs_review_valid
    needs_review_valid -- true ---> remove_other_labels
    needs_review_valid -- false ---> is_draft
    is_draft -- yes ---> mark_as_ready
    is_draft -- no ---> warning_about_label_addition
    mark_as_ready --> remove_other_labels
```


The warning is posted as a comment which will be deleted after 5
minutes.




```mermaid
---
title: needs review valid?
---
flowchart LR

%% vertices

    needs_review_valid([needs review\n valid?])
    true([true])
    false([false])
    latest_review_by_actor{latest review\n by actor}
    needs_work_valid[[needs work\n valid?]]
    positive_review_valid[[positive review\n valid?]]
    is_draft{is PR\n a draft?}

%% edges

    needs_review_valid ---> is_draft
    is_draft -- yes ---> false
    is_draft -- no ---> latest_review_by_actor
    latest_review_by_actor -- yes ---> true
    latest_review_by_actor -- no ---> needs_work_valid
    needs_work_valid -- true ---> false
    needs_work_valid -- false ---> positive_review_valid
    positive_review_valid -- true ---> false
    positive_review_valid -- false ---> true
```

```mermaid
---
title: needs work valid?
---
flowchart LR

%% vertices

    needs_work_valid([needs work\n valid?])
    true([true])
    false([false])
    latest_review_request_changes{latest proper\n review requests\n
changes?}

%% edges

    needs_work_valid  --> latest_review_request_changes
    latest_review_request_changes -- yes ---> true
    latest_review_request_changes -- no ---> false
```


```mermaid
---
title: positive review valid?
---
flowchart LR

%% vertices

    positive_review_valid([positive review\n valid?])
    true([true])
    false([false])
    latest_proper_review_approved{latest proper\n review\n approved?}

%% edges

    positive_review_valid  --> latest_proper_review_approved
    latest_proper_review_approved -- yes ---> true
    latest_proper_review_approved -- no ---> false
```

Here, proper means that the review is more than a comment.

##### What happens when adding s: needs work to a PR?


```mermaid
---
title: add the needs work label
---
flowchart LR

%% vertices

    trigger([label\n 's: needs work'\n added])
    request_changes([request changes])
    remove_other_labels([remove other\n state labels])
    warning_about_label_addition([warning\n about label\n addition])
    needs_work_valid[[needs work\n valid?]]
    is_draft{is PR\n a draft?}

%% edges

    trigger --> needs_work_valid
    needs_work_valid -- true ---> remove_other_labels
    needs_work_valid -- false ---> is_draft
    is_draft -- yes ---> warning_about_label_addition
    is_draft -- no ---> request_changes
    request_changes --> remove_other_labels
```

##### What happens when adding `s: positive review` to a PR?

```mermaid
---
title: add the positive review label
---
flowchart LR

%% vertices

    trigger([label\n 's: positive review'\n added])
    positive_review_valid[[positive review\n valid?]]
    approve_pr([approve])
    remove_other_labels([remove other\n state labels])
    actor_valid[[actor valid?]]
    approve_allowed[[approve\n allowed?]]
    warning_about_label_addition([warning\n about label\n addition])
    reject_label_addition[[reject\n label\n addition]]

%% edges

    trigger --> positive_review_valid
    positive_review_valid -- yes ---> remove_other_labels
    positive_review_valid -- no ---> actor_valid
    actor_valid -- yes ---> approve_allowed
    actor_valid -- no ---> reject_label_addition
    approve_allowed -- yes ---> approve_pr
    approve_allowed -- no ---> warning_about_label_addition
    approve_pr --> remove_other_labels
```

The boxes `actor_valid` and `reject_label_addition` are unchanged.


```mermaid
---
title: approve allowed?
---
flowchart LR

%% vertices

    trigger([approve\n allowed?])
    true([true])
    false([false])
    review_of_others_request_changes{changes\n requested by\n someone\n
else exists?}

%% edges

    trigger --> review_of_others_request_changes
    review_of_others_request_changes -- yes ---> false
    review_of_others_request_changes -- no ---> true
```

Here, only reviews of someone else are considered which are more recent
than any commit.



##### What happens when a state label is added to an issue?


```mermaid
---
title: add a state label to an issue
---
flowchart LR

%% vertices

    trigger([label added])
    warning_about_label_addition([warning\n about label\n addition])
    nothing([do nothing])
    is_needs_info{is label\n 's: needs info'?}

%% edges

    trigger --> is_needs_info
    is_needs_info -- yes --->  nothing
    is_needs_info -- no --->  warning_about_label_addition
```

##### What happens when removing a state label from a PR?

```mermaid
---
title: remove a state label
---
flowchart LR

%% vertices

    trigger([label removed])
    nothing([do nothing])
    hint_about_label_removal([hint\n about label\n removal])
    other_state_label{other state\n labels exist?}
    is_needs_info{is label\n 's: needs info'?}

%% edges

    trigger --> other_state_label
    other_state_label -- yes ---> nothing
    other_state_label -- no ---> is_needs_info
    is_needs_info -- yes --->  nothing
    is_needs_info -- no ---> hint_about_label_removal
```

The hint is postet as a comment which will be deleted after 5 minutes.

##### What happens when adding a priority label?

```mermaid
---
title: add a priority label
---
flowchart LR

%% vertices

    trigger([label added])
    remove_other_labels([remove other\n priority labels])

%% edges

    trigger --> remove_other_labels
```

##### What happens when removing a priority label?


```mermaid
---
title: remove a priority label
---
flowchart LR

%% vertices

    trigger([label removed])
    nothing([do nothing])
    hint_about_label_removal([hint\n about label\n removal])
    other_prio_label{other priority\n labels exist?}

%% edges

    trigger --> other_prio_label
    other_prio_label -- yes ---> nothing
    other_prio_label -- no ---> hint_about_label_removal
```

##### What happens when a PR is approved?

```mermaid
---
title: approve
---
flowchart LR

%% vertices

    trigger([approve\n])
    select_positive_review(['s: positive review'\n label selected])
    nothing([do nothing])
    positive_review_valid[[positive review\n valid?]]
    pending_review_requests{pending\n review requests\n exists?}
    actor_authorized{is actor\n a member of\n Triage?}

%% edges

    trigger ---> pending_review_requests
    pending_review_requests -- yes ---> nothing
    pending_review_requests -- no ---> actor_authorized
    actor_authorized -- yes ---> positive_review_valid
    actor_authorized -- no ---> nothing
    positive_review_valid -- true ---> select_positive_review
    positive_review_valid -- false ---> nothing
```


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36292
Reported by: Sebastian Oehms
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 28, 2023
…tion

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

Unfortunately the fix in sagemath#36213 has
not been completed properly. There is still a `return` from the
`get_review_decision` method which is not `None` where it should be.

This is fixed here. Sorry for this unnecessary trouble and my lack of
attention!

In a next step this PR fixes a bug which has been noticed by an
accidentally activation of the `unlabeled` trigger (see
sagemath#36292 (comment)).
Simultanously the `labeled`-trigger has been disabled. This is an
unrealistic setting which was not covered by the code.

Explicitely, a state label could not be removed even though other state
labels where set (examples  sagemath#36128
and sagemath#36020). This PR leads to the
following changes of behavior:

1. The reaction on the `unlabeled`-event is reduced to the case of the
last label
2. The `unlabeled`-event will not lead to a rejection of the removal any
more
3. Instead of a warning comment a hint comment is posted.

We found a way to test the bot a little more realistically (see
soehms#11). Additional problems emerged and
were also resolved. This led to the following changes in the workflow:


4. A bug in `actor_valid` is fixed (see
soehms#10 (comment))
5. Preparing to use our own bot user (e.g.
[sagemathadmins](https://github.com/sagemathadmins)) (see
soehms#11 (comment))
6. Waiver of observing the `reviewDecision` feature provided by GitHub
(see 2. in
soehms#11 (comment))
7. Allow the user to revert his decision of label selection (see
soehms#11 (comment))
8. Don't reject label addition any more, except in the case where the
author tries to set `s: positive review` to his own PR which has no
reviews from others (see
soehms#11 (comment))
9. Dismiss stale reviews of the bot after a push to the branch and on
submission of a new review which is more than just a comment (see
soehms#11 (comment)).

Despite the fact that the testing was now more realistic, it is still
not guaranteed that the bot's behavior in `sagemath/sage` will be
completely covered by our testing.

The changes in this PR cannot be activated immediately after merging
into the develop branch due to a bug in the GitHub web interface
observed during testing in soehms#11 (see
soehms#11 (comment) and
soehms#11 (comment)). The
problem is that the panel at the top right of the webpage that contains
the labels does not update immediately after the bot changes the labels.
Since this might cause confusion, I'll create a bug report about it. The
going-live of the sync bot will be stalled at least until we received an
answer.

This PR changes the following flowcharts from
sagemath#35172 :

##### What happens when adding `s: needs review` to a PR?

```mermaid
---
title: add the needs review label
---
flowchart LR

%% vertices

    trigger([label\n 's: needs review'\n added])
    mark_as_ready([mark as\n ready for\n review])
    remove_other_labels([remove other\n state labels])
    warning_about_label_addition([warning\n about label\n addition])
    needs_review_valid[[needs review\n valid?]]
    is_draft{is PR\n a draft?}

%% edges

    trigger --> needs_review_valid
    needs_review_valid -- true ---> remove_other_labels
    needs_review_valid -- false ---> is_draft
    is_draft -- yes ---> mark_as_ready
    is_draft -- no ---> warning_about_label_addition
    mark_as_ready --> remove_other_labels
```


The warning is posted as a comment which will be deleted after 5
minutes.




```mermaid
---
title: needs review valid?
---
flowchart LR

%% vertices

    needs_review_valid([needs review\n valid?])
    true([true])
    false([false])
    latest_review_by_actor{latest review\n by actor}
    needs_work_valid[[needs work\n valid?]]
    positive_review_valid[[positive review\n valid?]]
    is_draft{is PR\n a draft?}

%% edges

    needs_review_valid ---> is_draft
    is_draft -- yes ---> false
    is_draft -- no ---> latest_review_by_actor
    latest_review_by_actor -- yes ---> true
    latest_review_by_actor -- no ---> needs_work_valid
    needs_work_valid -- true ---> false
    needs_work_valid -- false ---> positive_review_valid
    positive_review_valid -- true ---> false
    positive_review_valid -- false ---> true
```

```mermaid
---
title: needs work valid?
---
flowchart LR

%% vertices

    needs_work_valid([needs work\n valid?])
    true([true])
    false([false])
    latest_review_request_changes{latest proper\n review requests\n
changes?}

%% edges

    needs_work_valid  --> latest_review_request_changes
    latest_review_request_changes -- yes ---> true
    latest_review_request_changes -- no ---> false
```


```mermaid
---
title: positive review valid?
---
flowchart LR

%% vertices

    positive_review_valid([positive review\n valid?])
    true([true])
    false([false])
    latest_proper_review_approved{latest proper\n review\n approved?}

%% edges

    positive_review_valid  --> latest_proper_review_approved
    latest_proper_review_approved -- yes ---> true
    latest_proper_review_approved -- no ---> false
```

Here, proper means that the review is more than a comment.

##### What happens when adding s: needs work to a PR?


```mermaid
---
title: add the needs work label
---
flowchart LR

%% vertices

    trigger([label\n 's: needs work'\n added])
    request_changes([request changes])
    remove_other_labels([remove other\n state labels])
    warning_about_label_addition([warning\n about label\n addition])
    needs_work_valid[[needs work\n valid?]]
    is_draft{is PR\n a draft?}

%% edges

    trigger --> needs_work_valid
    needs_work_valid -- true ---> remove_other_labels
    needs_work_valid -- false ---> is_draft
    is_draft -- yes ---> warning_about_label_addition
    is_draft -- no ---> request_changes
    request_changes --> remove_other_labels
```

##### What happens when adding `s: positive review` to a PR?

```mermaid
---
title: add the positive review label
---
flowchart LR

%% vertices

    trigger([label\n 's: positive review'\n added])
    positive_review_valid[[positive review\n valid?]]
    approve_pr([approve])
    remove_other_labels([remove other\n state labels])
    actor_valid[[actor valid?]]
    approve_allowed[[approve\n allowed?]]
    warning_about_label_addition([warning\n about label\n addition])
    reject_label_addition[[reject\n label\n addition]]

%% edges

    trigger --> positive_review_valid
    positive_review_valid -- yes ---> remove_other_labels
    positive_review_valid -- no ---> actor_valid
    actor_valid -- yes ---> approve_allowed
    actor_valid -- no ---> reject_label_addition
    approve_allowed -- yes ---> approve_pr
    approve_allowed -- no ---> warning_about_label_addition
    approve_pr --> remove_other_labels
```

The boxes `actor_valid` and `reject_label_addition` are unchanged.


```mermaid
---
title: approve allowed?
---
flowchart LR

%% vertices

    trigger([approve\n allowed?])
    true([true])
    false([false])
    review_of_others_request_changes{changes\n requested by\n someone\n
else exists?}

%% edges

    trigger --> review_of_others_request_changes
    review_of_others_request_changes -- yes ---> false
    review_of_others_request_changes -- no ---> true
```

Here, only reviews of someone else are considered which are more recent
than any commit.



##### What happens when a state label is added to an issue?


```mermaid
---
title: add a state label to an issue
---
flowchart LR

%% vertices

    trigger([label added])
    warning_about_label_addition([warning\n about label\n addition])
    nothing([do nothing])
    is_needs_info{is label\n 's: needs info'?}

%% edges

    trigger --> is_needs_info
    is_needs_info -- yes --->  nothing
    is_needs_info -- no --->  warning_about_label_addition
```

##### What happens when removing a state label from a PR?

```mermaid
---
title: remove a state label
---
flowchart LR

%% vertices

    trigger([label removed])
    nothing([do nothing])
    hint_about_label_removal([hint\n about label\n removal])
    other_state_label{other state\n labels exist?}
    is_needs_info{is label\n 's: needs info'?}

%% edges

    trigger --> other_state_label
    other_state_label -- yes ---> nothing
    other_state_label -- no ---> is_needs_info
    is_needs_info -- yes --->  nothing
    is_needs_info -- no ---> hint_about_label_removal
```

The hint is postet as a comment which will be deleted after 5 minutes.

##### What happens when adding a priority label?

```mermaid
---
title: add a priority label
---
flowchart LR

%% vertices

    trigger([label added])
    remove_other_labels([remove other\n priority labels])

%% edges

    trigger --> remove_other_labels
```

##### What happens when removing a priority label?


```mermaid
---
title: remove a priority label
---
flowchart LR

%% vertices

    trigger([label removed])
    nothing([do nothing])
    hint_about_label_removal([hint\n about label\n removal])
    other_prio_label{other priority\n labels exist?}

%% edges

    trigger --> other_prio_label
    other_prio_label -- yes ---> nothing
    other_prio_label -- no ---> hint_about_label_removal
```

##### What happens when a PR is approved?

```mermaid
---
title: approve
---
flowchart LR

%% vertices

    trigger([approve\n])
    select_positive_review(['s: positive review'\n label selected])
    nothing([do nothing])
    positive_review_valid[[positive review\n valid?]]
    pending_review_requests{pending\n review requests\n exists?}
    actor_authorized{is actor\n a member of\n Triage?}

%% edges

    trigger ---> pending_review_requests
    pending_review_requests -- yes ---> nothing
    pending_review_requests -- no ---> actor_authorized
    actor_authorized -- yes ---> positive_review_valid
    actor_authorized -- no ---> nothing
    positive_review_valid -- true ---> select_positive_review
    positive_review_valid -- false ---> nothing
```


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36292
Reported by: Sebastian Oehms
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 29, 2023
…tion

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

Unfortunately the fix in sagemath#36213 has
not been completed properly. There is still a `return` from the
`get_review_decision` method which is not `None` where it should be.

This is fixed here. Sorry for this unnecessary trouble and my lack of
attention!

In a next step this PR fixes a bug which has been noticed by an
accidentally activation of the `unlabeled` trigger (see
sagemath#36292 (comment)).
Simultanously the `labeled`-trigger has been disabled. This is an
unrealistic setting which was not covered by the code.

Explicitely, a state label could not be removed even though other state
labels where set (examples  sagemath#36128
and sagemath#36020). This PR leads to the
following changes of behavior:

1. The reaction on the `unlabeled`-event is reduced to the case of the
last label
2. The `unlabeled`-event will not lead to a rejection of the removal any
more
3. Instead of a warning comment a hint comment is posted.

We found a way to test the bot a little more realistically (see
soehms#11). Additional problems emerged and
were also resolved. This led to the following changes in the workflow:


4. A bug in `actor_valid` is fixed (see
soehms#10 (comment))
5. Preparing to use our own bot user (e.g.
[sagemathadmins](https://github.com/sagemathadmins)) (see
soehms#11 (comment))
6. Waiver of observing the `reviewDecision` feature provided by GitHub
(see 2. in
soehms#11 (comment))
7. Allow the user to revert his decision of label selection (see
soehms#11 (comment))
8. Don't reject label addition any more, except in the case where the
author tries to set `s: positive review` to his own PR which has no
reviews from others (see
soehms#11 (comment))
9. Dismiss stale reviews of the bot after a push to the branch and on
submission of a new review which is more than just a comment (see
soehms#11 (comment)).

Despite the fact that the testing was now more realistic, it is still
not guaranteed that the bot's behavior in `sagemath/sage` will be
completely covered by our testing.

The changes in this PR cannot be activated immediately after merging
into the develop branch due to a bug in the GitHub web interface
observed during testing in soehms#11 (see
soehms#11 (comment) and
soehms#11 (comment)). The
problem is that the panel at the top right of the webpage that contains
the labels does not update immediately after the bot changes the labels.
Since this might cause confusion, I'll create a bug report about it. The
going-live of the sync bot will be stalled at least until we received an
answer.

This PR changes the following flowcharts from
sagemath#35172 :

##### What happens when adding `s: needs review` to a PR?

```mermaid
---
title: add the needs review label
---
flowchart LR

%% vertices

    trigger([label\n 's: needs review'\n added])
    mark_as_ready([mark as\n ready for\n review])
    remove_other_labels([remove other\n state labels])
    warning_about_label_addition([warning\n about label\n addition])
    needs_review_valid[[needs review\n valid?]]
    is_draft{is PR\n a draft?}

%% edges

    trigger --> needs_review_valid
    needs_review_valid -- true ---> remove_other_labels
    needs_review_valid -- false ---> is_draft
    is_draft -- yes ---> mark_as_ready
    is_draft -- no ---> warning_about_label_addition
    mark_as_ready --> remove_other_labels
```


The warning is posted as a comment which will be deleted after 5
minutes.




```mermaid
---
title: needs review valid?
---
flowchart LR

%% vertices

    needs_review_valid([needs review\n valid?])
    true([true])
    false([false])
    latest_review_by_actor{latest review\n by actor}
    needs_work_valid[[needs work\n valid?]]
    positive_review_valid[[positive review\n valid?]]
    is_draft{is PR\n a draft?}

%% edges

    needs_review_valid ---> is_draft
    is_draft -- yes ---> false
    is_draft -- no ---> latest_review_by_actor
    latest_review_by_actor -- yes ---> true
    latest_review_by_actor -- no ---> needs_work_valid
    needs_work_valid -- true ---> false
    needs_work_valid -- false ---> positive_review_valid
    positive_review_valid -- true ---> false
    positive_review_valid -- false ---> true
```

```mermaid
---
title: needs work valid?
---
flowchart LR

%% vertices

    needs_work_valid([needs work\n valid?])
    true([true])
    false([false])
    latest_review_request_changes{latest proper\n review requests\n
changes?}

%% edges

    needs_work_valid  --> latest_review_request_changes
    latest_review_request_changes -- yes ---> true
    latest_review_request_changes -- no ---> false
```


```mermaid
---
title: positive review valid?
---
flowchart LR

%% vertices

    positive_review_valid([positive review\n valid?])
    true([true])
    false([false])
    latest_proper_review_approved{latest proper\n review\n approved?}

%% edges

    positive_review_valid  --> latest_proper_review_approved
    latest_proper_review_approved -- yes ---> true
    latest_proper_review_approved -- no ---> false
```

Here, proper means that the review is more than a comment.

##### What happens when adding s: needs work to a PR?


```mermaid
---
title: add the needs work label
---
flowchart LR

%% vertices

    trigger([label\n 's: needs work'\n added])
    request_changes([request changes])
    remove_other_labels([remove other\n state labels])
    warning_about_label_addition([warning\n about label\n addition])
    needs_work_valid[[needs work\n valid?]]
    is_draft{is PR\n a draft?}

%% edges

    trigger --> needs_work_valid
    needs_work_valid -- true ---> remove_other_labels
    needs_work_valid -- false ---> is_draft
    is_draft -- yes ---> warning_about_label_addition
    is_draft -- no ---> request_changes
    request_changes --> remove_other_labels
```

##### What happens when adding `s: positive review` to a PR?

```mermaid
---
title: add the positive review label
---
flowchart LR

%% vertices

    trigger([label\n 's: positive review'\n added])
    positive_review_valid[[positive review\n valid?]]
    approve_pr([approve])
    remove_other_labels([remove other\n state labels])
    actor_valid[[actor valid?]]
    approve_allowed[[approve\n allowed?]]
    warning_about_label_addition([warning\n about label\n addition])
    reject_label_addition[[reject\n label\n addition]]

%% edges

    trigger --> positive_review_valid
    positive_review_valid -- yes ---> remove_other_labels
    positive_review_valid -- no ---> actor_valid
    actor_valid -- yes ---> approve_allowed
    actor_valid -- no ---> reject_label_addition
    approve_allowed -- yes ---> approve_pr
    approve_allowed -- no ---> warning_about_label_addition
    approve_pr --> remove_other_labels
```

The boxes `actor_valid` and `reject_label_addition` are unchanged.


```mermaid
---
title: approve allowed?
---
flowchart LR

%% vertices

    trigger([approve\n allowed?])
    true([true])
    false([false])
    review_of_others_request_changes{changes\n requested by\n someone\n
else exists?}

%% edges

    trigger --> review_of_others_request_changes
    review_of_others_request_changes -- yes ---> false
    review_of_others_request_changes -- no ---> true
```

Here, only reviews of someone else are considered which are more recent
than any commit.



##### What happens when a state label is added to an issue?


```mermaid
---
title: add a state label to an issue
---
flowchart LR

%% vertices

    trigger([label added])
    warning_about_label_addition([warning\n about label\n addition])
    nothing([do nothing])
    is_needs_info{is label\n 's: needs info'?}

%% edges

    trigger --> is_needs_info
    is_needs_info -- yes --->  nothing
    is_needs_info -- no --->  warning_about_label_addition
```

##### What happens when removing a state label from a PR?

```mermaid
---
title: remove a state label
---
flowchart LR

%% vertices

    trigger([label removed])
    nothing([do nothing])
    hint_about_label_removal([hint\n about label\n removal])
    other_state_label{other state\n labels exist?}
    is_needs_info{is label\n 's: needs info'?}

%% edges

    trigger --> other_state_label
    other_state_label -- yes ---> nothing
    other_state_label -- no ---> is_needs_info
    is_needs_info -- yes --->  nothing
    is_needs_info -- no ---> hint_about_label_removal
```

The hint is postet as a comment which will be deleted after 5 minutes.

##### What happens when adding a priority label?

```mermaid
---
title: add a priority label
---
flowchart LR

%% vertices

    trigger([label added])
    remove_other_labels([remove other\n priority labels])

%% edges

    trigger --> remove_other_labels
```

##### What happens when removing a priority label?


```mermaid
---
title: remove a priority label
---
flowchart LR

%% vertices

    trigger([label removed])
    nothing([do nothing])
    hint_about_label_removal([hint\n about label\n removal])
    other_prio_label{other priority\n labels exist?}

%% edges

    trigger --> other_prio_label
    other_prio_label -- yes ---> nothing
    other_prio_label -- no ---> hint_about_label_removal
```

##### What happens when a PR is approved?

```mermaid
---
title: approve
---
flowchart LR

%% vertices

    trigger([approve\n])
    select_positive_review(['s: positive review'\n label selected])
    nothing([do nothing])
    positive_review_valid[[positive review\n valid?]]
    pending_review_requests{pending\n review requests\n exists?}
    actor_authorized{is actor\n a member of\n Triage?}

%% edges

    trigger ---> pending_review_requests
    pending_review_requests -- yes ---> nothing
    pending_review_requests -- no ---> actor_authorized
    actor_authorized -- yes ---> positive_review_valid
    actor_authorized -- no ---> nothing
    positive_review_valid -- true ---> select_positive_review
    positive_review_valid -- false ---> nothing
```


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36292
Reported by: Sebastian Oehms
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 29, 2023
…tion

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

Unfortunately the fix in sagemath#36213 has
not been completed properly. There is still a `return` from the
`get_review_decision` method which is not `None` where it should be.

This is fixed here. Sorry for this unnecessary trouble and my lack of
attention!

In a next step this PR fixes a bug which has been noticed by an
accidentally activation of the `unlabeled` trigger (see
sagemath#36292 (comment)).
Simultanously the `labeled`-trigger has been disabled. This is an
unrealistic setting which was not covered by the code.

Explicitely, a state label could not be removed even though other state
labels where set (examples  sagemath#36128
and sagemath#36020). This PR leads to the
following changes of behavior:

1. The reaction on the `unlabeled`-event is reduced to the case of the
last label
2. The `unlabeled`-event will not lead to a rejection of the removal any
more
3. Instead of a warning comment a hint comment is posted.

We found a way to test the bot a little more realistically (see
soehms#11). Additional problems emerged and
were also resolved. This led to the following changes in the workflow:


4. A bug in `actor_valid` is fixed (see
soehms#10 (comment))
5. Preparing to use our own bot user (e.g.
[sagemathadmins](https://github.com/sagemathadmins)) (see
soehms#11 (comment))
6. Waiver of observing the `reviewDecision` feature provided by GitHub
(see 2. in
soehms#11 (comment))
7. Allow the user to revert his decision of label selection (see
soehms#11 (comment))
8. Don't reject label addition any more, except in the case where the
author tries to set `s: positive review` to his own PR which has no
reviews from others (see
soehms#11 (comment))
9. Dismiss stale reviews of the bot after a push to the branch and on
submission of a new review which is more than just a comment (see
soehms#11 (comment)).

Despite the fact that the testing was now more realistic, it is still
not guaranteed that the bot's behavior in `sagemath/sage` will be
completely covered by our testing.

The changes in this PR cannot be activated immediately after merging
into the develop branch due to a bug in the GitHub web interface
observed during testing in soehms#11 (see
soehms#11 (comment) and
soehms#11 (comment)). The
problem is that the panel at the top right of the webpage that contains
the labels does not update immediately after the bot changes the labels.
Since this might cause confusion, I'll create a bug report about it. The
going-live of the sync bot will be stalled at least until we received an
answer.

This PR changes the following flowcharts from
sagemath#35172 :

##### What happens when adding `s: needs review` to a PR?

```mermaid
---
title: add the needs review label
---
flowchart LR

%% vertices

    trigger([label\n 's: needs review'\n added])
    mark_as_ready([mark as\n ready for\n review])
    remove_other_labels([remove other\n state labels])
    warning_about_label_addition([warning\n about label\n addition])
    needs_review_valid[[needs review\n valid?]]
    is_draft{is PR\n a draft?}

%% edges

    trigger --> needs_review_valid
    needs_review_valid -- true ---> remove_other_labels
    needs_review_valid -- false ---> is_draft
    is_draft -- yes ---> mark_as_ready
    is_draft -- no ---> warning_about_label_addition
    mark_as_ready --> remove_other_labels
```


The warning is posted as a comment which will be deleted after 5
minutes.




```mermaid
---
title: needs review valid?
---
flowchart LR

%% vertices

    needs_review_valid([needs review\n valid?])
    true([true])
    false([false])
    latest_review_by_actor{latest review\n by actor}
    needs_work_valid[[needs work\n valid?]]
    positive_review_valid[[positive review\n valid?]]
    is_draft{is PR\n a draft?}

%% edges

    needs_review_valid ---> is_draft
    is_draft -- yes ---> false
    is_draft -- no ---> latest_review_by_actor
    latest_review_by_actor -- yes ---> true
    latest_review_by_actor -- no ---> needs_work_valid
    needs_work_valid -- true ---> false
    needs_work_valid -- false ---> positive_review_valid
    positive_review_valid -- true ---> false
    positive_review_valid -- false ---> true
```

```mermaid
---
title: needs work valid?
---
flowchart LR

%% vertices

    needs_work_valid([needs work\n valid?])
    true([true])
    false([false])
    latest_review_request_changes{latest proper\n review requests\n
changes?}

%% edges

    needs_work_valid  --> latest_review_request_changes
    latest_review_request_changes -- yes ---> true
    latest_review_request_changes -- no ---> false
```


```mermaid
---
title: positive review valid?
---
flowchart LR

%% vertices

    positive_review_valid([positive review\n valid?])
    true([true])
    false([false])
    latest_proper_review_approved{latest proper\n review\n approved?}

%% edges

    positive_review_valid  --> latest_proper_review_approved
    latest_proper_review_approved -- yes ---> true
    latest_proper_review_approved -- no ---> false
```

Here, proper means that the review is more than a comment.

##### What happens when adding s: needs work to a PR?


```mermaid
---
title: add the needs work label
---
flowchart LR

%% vertices

    trigger([label\n 's: needs work'\n added])
    request_changes([request changes])
    remove_other_labels([remove other\n state labels])
    warning_about_label_addition([warning\n about label\n addition])
    needs_work_valid[[needs work\n valid?]]
    is_draft{is PR\n a draft?}

%% edges

    trigger --> needs_work_valid
    needs_work_valid -- true ---> remove_other_labels
    needs_work_valid -- false ---> is_draft
    is_draft -- yes ---> warning_about_label_addition
    is_draft -- no ---> request_changes
    request_changes --> remove_other_labels
```

##### What happens when adding `s: positive review` to a PR?

```mermaid
---
title: add the positive review label
---
flowchart LR

%% vertices

    trigger([label\n 's: positive review'\n added])
    positive_review_valid[[positive review\n valid?]]
    approve_pr([approve])
    remove_other_labels([remove other\n state labels])
    actor_valid[[actor valid?]]
    approve_allowed[[approve\n allowed?]]
    warning_about_label_addition([warning\n about label\n addition])
    reject_label_addition[[reject\n label\n addition]]

%% edges

    trigger --> positive_review_valid
    positive_review_valid -- yes ---> remove_other_labels
    positive_review_valid -- no ---> actor_valid
    actor_valid -- yes ---> approve_allowed
    actor_valid -- no ---> reject_label_addition
    approve_allowed -- yes ---> approve_pr
    approve_allowed -- no ---> warning_about_label_addition
    approve_pr --> remove_other_labels
```

The boxes `actor_valid` and `reject_label_addition` are unchanged.


```mermaid
---
title: approve allowed?
---
flowchart LR

%% vertices

    trigger([approve\n allowed?])
    true([true])
    false([false])
    review_of_others_request_changes{changes\n requested by\n someone\n
else exists?}

%% edges

    trigger --> review_of_others_request_changes
    review_of_others_request_changes -- yes ---> false
    review_of_others_request_changes -- no ---> true
```

Here, only reviews of someone else are considered which are more recent
than any commit.



##### What happens when a state label is added to an issue?


```mermaid
---
title: add a state label to an issue
---
flowchart LR

%% vertices

    trigger([label added])
    warning_about_label_addition([warning\n about label\n addition])
    nothing([do nothing])
    is_needs_info{is label\n 's: needs info'?}

%% edges

    trigger --> is_needs_info
    is_needs_info -- yes --->  nothing
    is_needs_info -- no --->  warning_about_label_addition
```

##### What happens when removing a state label from a PR?

```mermaid
---
title: remove a state label
---
flowchart LR

%% vertices

    trigger([label removed])
    nothing([do nothing])
    hint_about_label_removal([hint\n about label\n removal])
    other_state_label{other state\n labels exist?}
    is_needs_info{is label\n 's: needs info'?}

%% edges

    trigger --> other_state_label
    other_state_label -- yes ---> nothing
    other_state_label -- no ---> is_needs_info
    is_needs_info -- yes --->  nothing
    is_needs_info -- no ---> hint_about_label_removal
```

The hint is postet as a comment which will be deleted after 5 minutes.

##### What happens when adding a priority label?

```mermaid
---
title: add a priority label
---
flowchart LR

%% vertices

    trigger([label added])
    remove_other_labels([remove other\n priority labels])

%% edges

    trigger --> remove_other_labels
```

##### What happens when removing a priority label?


```mermaid
---
title: remove a priority label
---
flowchart LR

%% vertices

    trigger([label removed])
    nothing([do nothing])
    hint_about_label_removal([hint\n about label\n removal])
    other_prio_label{other priority\n labels exist?}

%% edges

    trigger --> other_prio_label
    other_prio_label -- yes ---> nothing
    other_prio_label -- no ---> hint_about_label_removal
```

##### What happens when a PR is approved?

```mermaid
---
title: approve
---
flowchart LR

%% vertices

    trigger([approve\n])
    select_positive_review(['s: positive review'\n label selected])
    nothing([do nothing])
    positive_review_valid[[positive review\n valid?]]
    pending_review_requests{pending\n review requests\n exists?}
    actor_authorized{is actor\n a member of\n Triage?}

%% edges

    trigger ---> pending_review_requests
    pending_review_requests -- yes ---> nothing
    pending_review_requests -- no ---> actor_authorized
    actor_authorized -- yes ---> positive_review_valid
    actor_authorized -- no ---> nothing
    positive_review_valid -- true ---> select_positive_review
    positive_review_valid -- false ---> nothing
```


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36292
Reported by: Sebastian Oehms
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 4, 2023
…tic varieties

    
<!-- Describe your changes here in detail -->
 Recently in sagemath#36128 (already in develop) characteristic varieties of
finitely presented fundamental groups were introduced. Its computation
is based on Fitting ideals of Laurent polynomial matrices. In sagemath#36299,
Fitting ideals were implemented for generic rings with some improvements
for PID and polynomial rings.

There are two original goals in this PR: to improve the output of
characteristic varieties and to use the cited implementation.  In order
to make computations faster, the implementation of Fitting ideals should
apply to Laurent polynomial rings and for this goal, several changes
should be applied to Laurent polynomials in `Sagemath`.

I am not sure if a deeper change should be made, since I applied only
the changes I needed for the above goal:

- src/sage/groups/finitely_presented.py:
    - Changes in `fitting_ideals`.
- src/sage/matrix/matrix2.py: Style changes.
- src/sage/matrix/matrix_laurent_mpolynomial_dense.pyx: This is a new
file to create the class `Matrix_laurent_mpolynomial_dense`.
    - A method `laurent_matrix_reduction` to obtain a matrix of
polynomials where the variables are non common factors for neither the
rows nor the columns.
    - A methord `_fitting_ideal` to use the same method for matrices of
polynomials.
- src/sage/matrix/matrix__mpolynomial_dense.pyx: Style changes.

The main changes are for Laurent polynomials to avoid errors in the
above implementations.

- src/sage/rings/polynomials/laurent_polynomial.pyx:
    - Style changes.
    - Create `xgcd` needed for `inverse_mod`.
    - Create `inverse_mod`.
    - Create `divides`, I copied the code for polynomials with minor
changes.
- src/sage/rings/polynomials/laurent_polynomial_ideal.py:
    - Style changes.
    - Changes in hint keyword in  `__init__`, the previous code create
issues, e.g., impossible to sum ideals of univariate Laurent polynomial
rings. They involve changes in doctests for `hint`
    - Changes in `__contains__` since `__reduce__` is different for
univariate and multivaraite case.
    - Create `gens_reduced`.
    - Changes in `polynomial_ideal` to deal differently if uni- and
multi-variate.
- src/sage/rings/polynomials/laurent_polynomial_mpair.py:
    - Style changes.
    - Create `divides`, I copied the code for polynomials with minor
changes.
- src/sage/rings/polynomials/laurent_polynomial_ring.py:
    - Style changes.
    - Some `TestSuite`'s applied to domains as base_rings; the
corresponding `TestSuite`'s for polynomials also failed if applied to
polynomial rings.
- src/sage/rings/polynomials/laurent_polynomial_ring_base.py:
    - Style changes.
    - Implement `is_noetherian`.
- src/sage/rings/polynomials/polynomial_element.pyx: Style changes.

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36368
Reported by: Enrique Manuel Artal Bartolo
Reviewer(s): Enrique Manuel Artal Bartolo, kedlaya, miguelmarco, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 5, 2023
…tic varieties

    
<!-- Describe your changes here in detail -->
 Recently in sagemath#36128 (already in develop) characteristic varieties of
finitely presented fundamental groups were introduced. Its computation
is based on Fitting ideals of Laurent polynomial matrices. In sagemath#36299,
Fitting ideals were implemented for generic rings with some improvements
for PID and polynomial rings.

There are two original goals in this PR: to improve the output of
characteristic varieties and to use the cited implementation.  In order
to make computations faster, the implementation of Fitting ideals should
apply to Laurent polynomial rings and for this goal, several changes
should be applied to Laurent polynomials in `Sagemath`.

I am not sure if a deeper change should be made, since I applied only
the changes I needed for the above goal:

- src/sage/groups/finitely_presented.py:
    - Changes in `fitting_ideals`.
- src/sage/matrix/matrix2.py: Style changes.
- src/sage/matrix/matrix_laurent_mpolynomial_dense.pyx: This is a new
file to create the class `Matrix_laurent_mpolynomial_dense`.
    - A method `laurent_matrix_reduction` to obtain a matrix of
polynomials where the variables are non common factors for neither the
rows nor the columns.
    - A methord `_fitting_ideal` to use the same method for matrices of
polynomials.
- src/sage/matrix/matrix__mpolynomial_dense.pyx: Style changes.

The main changes are for Laurent polynomials to avoid errors in the
above implementations.

- src/sage/rings/polynomials/laurent_polynomial.pyx:
    - Style changes.
    - Create `xgcd` needed for `inverse_mod`.
    - Create `inverse_mod`.
    - Create `divides`, I copied the code for polynomials with minor
changes.
- src/sage/rings/polynomials/laurent_polynomial_ideal.py:
    - Style changes.
    - Changes in hint keyword in  `__init__`, the previous code create
issues, e.g., impossible to sum ideals of univariate Laurent polynomial
rings. They involve changes in doctests for `hint`
    - Changes in `__contains__` since `__reduce__` is different for
univariate and multivaraite case.
    - Create `gens_reduced`.
    - Changes in `polynomial_ideal` to deal differently if uni- and
multi-variate.
- src/sage/rings/polynomials/laurent_polynomial_mpair.py:
    - Style changes.
    - Create `divides`, I copied the code for polynomials with minor
changes.
- src/sage/rings/polynomials/laurent_polynomial_ring.py:
    - Style changes.
    - Some `TestSuite`'s applied to domains as base_rings; the
corresponding `TestSuite`'s for polynomials also failed if applied to
polynomial rings.
- src/sage/rings/polynomials/laurent_polynomial_ring_base.py:
    - Style changes.
    - Implement `is_noetherian`.
- src/sage/rings/polynomials/polynomial_element.pyx: Style changes.

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36368
Reported by: Enrique Manuel Artal Bartolo
Reviewer(s): Enrique Manuel Artal Bartolo, kedlaya, miguelmarco, Travis Scrimshaw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants