Skip to content

Conversation

@adnanbhat7
Copy link

@adnanbhat7 adnanbhat7 commented Nov 16, 2025

1.Changed primary parameter from impl to implementation in docstrings
2.Marked impl as deprecated (backwards compatibility)
3.Updated examples to use implementation instead of impl
4.Added examples showing both parameters work
5. clean commit history
@nbruin review this

📝 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

related to #41123

Copy link
Member

@vincentmacri vincentmacri left a comment

Choose a reason for hiding this comment

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

Can you add "Fixes #40806" to the PR description?

I have one small requested change for now. I'll take a closer look next week if nobody else gets to reviewing this first. And same comment I made on your last PR still applies:

Since you're a first-time contributor GitHub permissions make running the CI kind of annoying. To speed things up, please run at least the ruff linter locally and make sure our required rules are passing on any files you've changed before pushing your changes (see https://doc.sagemath.org/html/en/developer/tools.html for how to run ruff locally)

After you make the requested change and verify that the linter is passing locally, tag sagemath/core to ask for the CI run to be approved.

"""
for key, val in kwds.items():
if key not in ['structure', 'implementation', 'prec', 'embedding', 'latex_names']:
if key not in ['structure', 'implementation', 'prec', 'embedding', 'latex_names', 'impl']:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if key not in ['structure', 'implementation', 'prec', 'embedding', 'latex_names', 'impl']:
if key not in ['structure', 'prec', 'embedding', 'latex_names', 'impl']:

Not needed since implementation is an explicit keyword argument.

@adnanbhat7
Copy link
Author

fixed the implemntaion that was present in kwds also ran ruff locally all checks passed
Screenshot 2025-11-19 234408
@sagemath/core i need CI run

@vincentmacri
Copy link
Member

@rgabhi2526 pointed out #35609 which was also implementing this but stalled. Looks like there were more substantial changes there, I'll need to look into if those are needed here. If so we should add those commits in a way that @Newtech66 gets credit for the previous work in the commit history (I'll need to look up the git commands for that).

@cxzhong
Copy link
Contributor

cxzhong commented Nov 20, 2025

@roed314 please run this workflow, thank you

@cxzhong
Copy link
Contributor

cxzhong commented Nov 20, 2025

@rgabhi2526 pointed out #35609 which was also implementing this but stalled. Looks like there were more substantial changes there, I'll need to look into if those are needed here. If so we should add those commits in a way that @Newtech66 gets credit for the previous work in the commit history (I'll need to look up the git commands for that).

If we can cherry-pick #35609 directly?

@vincentmacri
Copy link
Member

If we can cherry-pick #35609 directly?

Do you mean as a separate PR or as part of this PR? Either way, need to figure out which commits to cherry-pick. I think the changes to the main function in this PR are a bit cleaner than in the older PR, but the older PR has a lot of documentation updates that are missing from this one. Those are minor though, so it might be easier to defer cherry-picking to a followup PR.

The other PR also made a change somewhere in categories because the way they fixed it exposed some other bug. We need to check if that's an issue here (I'm guessing that's why the tests failed, but I need to check).

@vincentmacri
Copy link
Member

@adnanbhat7 I opened adnanbhat7#1 in your repo to cherry-pick changes from the related #35609 and reconcile the resulting merge conflicts. Also some minor cleanup to your stuff.

I've marked it as a draft PR until I finish running tests locally, once I verify that it works locally I'll unmark it as a draft and if you agree with the changes I'm suggesting then you can merge my PR into your branch, which will then update this PR.

With the amount of changes I'm suggesting (even though they are actually just cherry-picking from someone else's work), a second person should probably approve this. @cxzhong can you review this once it's ready?

@vincentmacri
Copy link
Member

Everything is passing locally.

@roed314 Can we get another CI run?

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.

4 participants