Skip to content

Comments

sage.schemes: Update # needs#36102

Merged
vbraun merged 295 commits intosagemath:developfrom
mkoeppe:needs_for_sage_schemes
Sep 10, 2023
Merged

sage.schemes: Update # needs#36102
vbraun merged 295 commits intosagemath:developfrom
mkoeppe:needs_for_sage_schemes

Conversation

@mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Aug 18, 2023 •
edited
Loading

📝 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

@enriqueartal
Merge branch 'u/enriqueartal/sirocco_improvements__discriminant_and_o…
299fc2d 
…ptional_information' of trac.sagemath.org:sage into t/34415/sirocco_improvements__discriminant_and_optional_information
@enriqueartal
Starting Puiseux ...
0d0fc48
@enriqueartal
Some improvements including fast comparing algebraic numbers
50b5713
@enriqueartal
cleaning
5de0b06
@enriqueartal
going back to compare algebraic numbers
a6282e5
@enriqueartal
going back to compare algebraic numbers
3e8531e
@enriqueartal
going back to compare algebraic numbers
764d1d2
@enriqueartal
branch puiseux
c38214b
@enriqueartal
typos for test
709ca9a
@enriqueartal
tests
3ca1c98
@enriqueartal
improving relationship of strands and components
f489573
@enriqueartal
improving relationship of strands and components
2767a22
@enriqueartal
lint corrections
89e80ee
@enriqueartal
lint corrections
598ce7f
@enriqueartal
adding optional arguments
73a555b
@enriqueartal
adding optional arguments
bd51861
@enriqueartal
bad merge
e9b313a
@enriqueartal
Add more functions and methods
052a2c2
@enriqueartal
discrim was too long
3ab76f9
@enriqueartal
discrim was too long
7b05103
@enriqueartal
fusion
fede822
@enriqueartal
delete some print
889daf9
@enriqueartal
Merge branch 'u/enriqueartal/sirocco_improvements__discriminant_and_o…
96ad5e9 
…ptional_information' of ssh://trac.sagemath.org/sage into t/34415/sirocco_improvements__discriminant_and_optional_information
@enriqueartal
Merge branch 'u/enriqueartal/sirocco_improvements__discriminant_and_o…
27cba50 
…ptional_information' of ssh://trac.sagemath.org/sage into t/34415/sirocco_improvements__discriminant_and_optional_information
@enriqueartal
Merge branch 'develop' into t/34415/sirocco_improvements__discriminan…
81131ce 
…t_and_optional_information
@enriqueartal
Merge branch 'develop' of https://github.com/sagemath/sage into develop
c4dbcd9
@enriqueartal
Merge branch 'develop' into t/34415/sirocco_improvements__discriminan…
f1ccc2e 
…t_and_optional_information
@enriqueartal
Changes using more algebraic numbers; some tests do not hold
c0ed857
@enriqueartal
Merge branch 'develop' into t/34415/sirocco_improvements__discriminan…
47a7865 
…t_and_optional_information
@enriqueartal
modification of functions
7cbaf0f
@enriqueartal
Copy link
Contributor

enriqueartal commented Aug 29, 2023

The test ended with one failure (as in the first paragraph of #35376 (comment)). These are random errors (maybe related with parallelism) which appeared already in the previous version. The test in my computer works and run it in a loop (with no failure up to now) in order to understand why.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 29, 2023

Thanks @enriqueartal, I've merged the latest version of #35376

@enriqueartal
Copy link
Contributor

enriqueartal commented Aug 29, 2023 •
edited
Loading

I think the merge was not right.
Actually, yes, but this is not the checked version.
Edited again, it was the right version. I do not understand why the test failed. My computer runs Fedora 39 and the test is successful.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 29, 2023

This error:

sage -t --random-seed=127684187301711749641523852874958644185 src/sage/schemes/curves/zariski_vankampen.py
**********************************************************************
File "src/sage/schemes/curves/zariski_vankampen.py", line 1577, in sage.schemes.curves.zariski_vankampen.fundamental_group
Failed example:
    fundamental_group(f)
Exception raised:
    Traceback (most recent call last):
      File "/sage/src/sage/doctest/forker.py", line 709, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/sage/src/sage/doctest/forker.py", line 1144, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.schemes.curves.zariski_vankampen.fundamental_group[7]>", line 1, in <module>
        fundamental_group(f)
      File "/sage/src/sage/schemes/curves/zariski_vankampen.py", line 1608, in fundamental_group
        bm = braid_monodromy(g)[0]
             ^^^^^^^^^^^^^^^^^^
      File "/sage/src/sage/schemes/curves/zariski_vankampen.py", line 1243, in braid_monodromy
        segsbraids[(endseg, beginseg)] = b.inverse()
                                         ^^^^^^^^^
    AttributeError: 'str' object has no attribute 'inverse'
**********************************************************************
1 item had failures:
   1 of   9 in sage.schemes.curves.zariski_vankampen.fundamental_group

... means that ChildProcessError is not the only way how reading from sirocco can fail (?).
Catch more exceptions?

@enriqueartal
Copy link
Contributor

enriqueartal commented Aug 30, 2023

I made some changes to be more verbose and there is also an AtributeError which does not occur at this place but in followstrand or roots_interval. There is also a point, the errors occur only in the incremental build not in the total one.

@enriqueartal
Copy link
Contributor

enriqueartal commented Aug 30, 2023

As usual, it was a stupid typo. I wrote sage optional - sirocco instead of sage: optional - sirocco in the failing block. Now it works, sorry for the noise.

kwankyu
kwankyu reviewed Aug 30, 2023
View reviewed changes
kwankyu
kwankyu reviewed Aug 30, 2023
View reviewed changes
kwankyu
kwankyu reviewed Aug 30, 2023
View reviewed changes
@kwankyu
Copy link
Collaborator

kwankyu commented Aug 30, 2023

@mkoeppe may take care of my new comments. Otherwise, lgtm.

@enriqueartal
Copy link
Contributor

enriqueartal commented Aug 31, 2023

I made the changes proposed by @kwankyu's in my branch. Thanks!

@github-actions
Copy link

github-actions bot commented Aug 31, 2023

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 1, 2023

I've merged the changes. All tests pass

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 1, 2023

Thanks. LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 1, 2023

Thank you!

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 2, 2023
bce3d3c 
sagemathgh-36102: `sage.schemes`: Update `# needs`
    
<!-- ^^^^^
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? -->
- Part of: sagemath#29705
- Cherry-picked from: sagemath#35095
<!-- 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.
- [ ] 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: ...
-->
- Depends on sagemath#35376 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36102
Reported by: Matthias Köppe
Reviewer(s): David Coudert, Enrique Manuel Artal Bartolo, Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 4, 2023
025f479 
sagemathgh-36102: `sage.schemes`: Update `# needs`
    
<!-- ^^^^^
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? -->
- Part of: sagemath#29705
- Cherry-picked from: sagemath#35095
<!-- 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.
- [ ] 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: ...
-->
- Depends on sagemath#35376 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36102
Reported by: Matthias Köppe
Reviewer(s): David Coudert, Enrique Manuel Artal Bartolo, Kwankyu Lee, Matthias Köppe
@dcoudert dcoudert mentioned this pull request Sep 4, 2023
Merged
2 tasks
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 5, 2023
2a4953e 
sagemathgh-36102: `sage.schemes`: Update `# needs`
    
<!-- ^^^^^
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? -->
- Part of: sagemath#29705
- Cherry-picked from: sagemath#35095
<!-- 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.
- [ ] 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: ...
-->
- Depends on sagemath#35376 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36102
Reported by: Matthias Köppe
Reviewer(s): David Coudert, Enrique Manuel Artal Bartolo, Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 10, 2023
sagemathgh-36102: sage.schemes: Update # needs
00c3f9a 
<!-- ^^^^^
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? -->
- Part of: sagemath#29705
- Cherry-picked from: sagemath#35095
<!-- 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.
- [ ] 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: ...
-->
- Depends on sagemath#35376 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

URL: sagemath#36102
Reported by: Matthias Köppe
Reviewer(s): David Coudert, Enrique Manuel Artal Bartolo, Kwankyu Lee, Matthias Köppe
@vbraun vbraun merged commit aa1cfd7 into sagemath:develop Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kwankyu kwankyu kwankyu approved these changes

+2 more reviewers

@dcoudert dcoudert dcoudert approved these changes

@enriqueartal enriqueartal enriqueartal left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mkoeppe mkoeppe

Labels

c: algebraic geometry t: refactoring

Projects

None yet

Milestone

sage-10.2

Development

Successfully merging this pull request may close these issues.

5 participants

@mkoeppe @dcoudert @kwankyu @vbraun @enriqueartal