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

use Parent in Monsky-Washnitzer #37079

Merged
merged 3 commits into from
Jan 22, 2024
Merged

Conversation

fchapoton
Copy link
Contributor

use Parent in the modified file

also removing __neg__ method

and some other details

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.

@fchapoton
Copy link
Contributor Author

The TestSuite is not there, as it fails on pickling..

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.

In addition to the comments below, can you explain a bit about why you removed the __neg__? The generic implementation seems like it would be slower (I don't know how much that would matter though).

Should we also try and fix the pickling issue to? Could you say at least a bit more about the failure? In particular, on which class(es)?

@fchapoton
Copy link
Contributor Author

TestSuite in fact seem to be all ok. I added one missing and it works.

About __neg__, it was a source of failure, and so I took the simplest path to make it work : removal.

I have removed the comments and added one UniqueRepresentation.

@tscrim
Copy link
Collaborator

tscrim commented Jan 18, 2024

TestSuite in fact seem to be all ok. I added one missing and it works.

Was it failing before the UniqueRepresentation was added? That might have fixed some things... Well, not really anything worth spending time on.

About __neg__, it was a source of failure, and so I took the simplest path to make it work : removal.

This is very strange to me. It very clearly should work. I am worried it is hiding a bug. Can you say a bit more about the failures?

I have removed the comments and added one UniqueRepresentation.

Thank you.

PS - Did you also get my email about March?

@fchapoton
Copy link
Contributor Author

ok, so I have added back __neg__ and it works.

Sorry for the unanswered mail. I will do that now.

Copy link

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

@tscrim
Copy link
Collaborator

tscrim commented Jan 19, 2024

ok, so I have added back __neg__ and it works.

That is just as strange. Well, I am happy that it is working as I would have expected it to. Thank you. This is now a positive review.

Sorry for the unanswered mail. I will do that now.

No problem. Thank you. I wasn't sure if it ended up in a spam folder or was some outdated email (Japanese universities seem to enjoy changing email addresses every few years in some cases...).

@vbraun vbraun merged commit e788164 into sagemath:develop Jan 22, 2024
18 of 20 checks passed
@fchapoton fchapoton deleted the parent-monsky branch January 22, 2024 08:33
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
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

4 participants