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

Add subs method for function field elements #38607

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

vincentmacri
Copy link
Contributor

@vincentmacri vincentmacri commented Sep 3, 2024

Fixes #38514.

📝 Checklist

  • The title is concise and informative.
  • 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 and checked the documentation preview.

⌛ Dependencies

None

Copy link

github-actions bot commented Sep 3, 2024

Documentation preview for this PR (built with commit 6a34481; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@vincentmacri
Copy link
Contributor Author

I'm not really sure what the rules/conventions are around using # needs sage.[whatever] in doc tests. I didn't use it in any of my tests because I wasn't really sure how/why it's used. Any feedback/guidance on that would be appreciated.

@vincentmacri vincentmacri marked this pull request as ready for review September 3, 2024 18:47
@vincentmacri
Copy link
Contributor Author

@DaveWitteMorris could I get a review? (Asking since you triaged the bug report.)

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.

Here are my suggestions; please let me know if you have any questions.

src/sage/rings/function_field/element.pyx Outdated Show resolved Hide resolved
src/sage/rings/function_field/element.pyx Outdated Show resolved Hide resolved
src/sage/rings/function_field/element.pyx Outdated Show resolved Hide resolved
src/sage/rings/function_field/element.pyx Outdated Show resolved Hide resolved
src/sage/rings/function_field/element.pyx Outdated Show resolved Hide resolved
vincentmacri and others added 2 commits September 16, 2024 09:23
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@vincentmacri
Copy link
Contributor Author

Thanks @tscrim! My only question is this one above:

I'm not really sure what the rules/conventions are around using # needs sage.[whatever] in doc tests. I didn't use it in any of my tests because I wasn't really sure how/why it's used. Any feedback/guidance on that would be appreciated.

@tscrim
Copy link
Collaborator

tscrim commented Sep 16, 2024

Thanks @tscrim! My only question is this one above:

I'm not really sure what the rules/conventions are around using # needs sage.[whatever] in doc tests. I didn't use it in any of my tests because I wasn't really sure how/why it's used. Any feedback/guidance on that would be appreciated.

Sorry, I forgot to answer that. Don’t worry about it; you do not need to add anything. It is for modularization that will be fixed by whomever knowns if something is necessary.

@vincentmacri
Copy link
Contributor Author

Great! In that case is this ready to merge? I think the CI failures are unrelated. The tests pass on my machine at least.

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.

Indeed, I think this can be merged.

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 22, 2024
    
Fixes sagemath#38514.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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 and checked the documentation
preview.

### ⌛ Dependencies

None
    
URL: sagemath#38607
Reported by: Vincent Macri
Reviewer(s): Travis Scrimshaw, Vincent Macri
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.

Implement subs or __call__ for FunctionFieldElement
2 participants