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

Choice of the name of primitive elements for subfields of QQbar #36749

Merged
merged 9 commits into from
Dec 14, 2023

Conversation

enriqueartal
Copy link
Contributor

Added optional keywords name for the method union and the function number_field_elements_from_algebraics from qqbar.py.

Without this option the name, the name is always a and it may conflict if a is another object, see https://groups.google.com/g/sage-devel/c/mr9AzpUcbUM

📝 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.

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.

Just some simple doc fixes. LGTM otherwise.

src/sage/rings/qqbar.py Outdated Show resolved Hide resolved
src/sage/rings/qqbar.py Outdated Show resolved Hide resolved
@fchapoton
Copy link
Contributor

Travis also suggested to remove the final dot in the description of parameters

@enriqueartal
Copy link
Contributor Author

Travis also suggested to remove the final dot in the description of parameters

Sorry, I read it too quickly. Done.

src/sage/rings/qqbar.py Outdated Show resolved Hide resolved
Correct length hyphen

Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
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.

Thanks. LGTM.

Whomever thought that a default commit message for some GH way of incorporating reviewer suggested changes should actually work on a project of any meaningful size and then if they cannot see the clear gravity of their error should be immediately fired.

Copy link

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

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 10, 2023
…lds of QQbar

    
<!-- Describe your changes here in detail -->
Added optional keywords `name` for the method `union` and the function
`number_field_elements_from_algebraics` from `qqbar.py`.

Without this option the name, the name is always `a` and it may conflict
if  `a` is another object, see https://groups.google.com/g/sage-
devel/c/mr9AzpUcbUM
### 📝 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.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.
    
URL: sagemath#36749
Reported by: Enrique Manuel Artal Bartolo
Reviewer(s): Travis Scrimshaw
@vbraun vbraun merged commit 11c5766 into sagemath:develop Dec 14, 2023
11 of 17 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 14, 2023
@enriqueartal enriqueartal deleted the numberfields branch December 16, 2023 17:49
@enriqueartal enriqueartal restored the numberfields branch December 16, 2023 17:49
@enriqueartal enriqueartal deleted the numberfields branch December 16, 2023 17:50
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.

None yet

6 participants