Skip to content

promote cleanup #3903

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

Draft
wants to merge 7 commits into
base: development
Choose a base branch
from
Draft

promote cleanup #3903

wants to merge 7 commits into from

Conversation

pzinn
Copy link
Contributor

@pzinn pzinn commented Jul 1, 2025

clean up promote code:

@pzinn pzinn marked this pull request as draft July 1, 2025 14:01
@@ -106,10 +106,13 @@ promote(RawRingElement,RingElement) := (x,R) -> new R from x
promote(Number,InexactNumber) := (x,RR) -> promote(x,default RR)
promote(ZZ,RR') :=
promote(QQ,RR') :=
promote(RR',RR') :=
Copy link
Member

Choose a reason for hiding this comment

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

A bit confused:

  1. I think there's a better way to have promote behave like identity if the input ring and new ring are identical.
  2. The commit that changes this file says

fix isPromotable ring family
This reverts commit 63d7fc7.

But that commit doesn't seem to have anything to do with this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the revert part of the message is an accident. will fix.
the addition of promote(XX',YY') is an unfortunate side-effect of the messy way that inexact fields are implemented in Macaulay2 (which one day I'd love to rewrite from scratch, because it's such a mess).
They're not actual functions though because they never actually get called, really one should make them dummy functions.

nottosamering := (X,Y) -> (
if X === Y then error("expected ",pluralsynonym X, " for compatible rings")
else error("expected ",synonym X," and ",synonym Y," for compatible rings"))
tosamering = (M,N) -> (
Copy link
Member

Choose a reason for hiding this comment

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

I don't mind keeping the old names if you don't want to change names in a bunch of places, but I don't think we should actively rename something camel-cased to all lower case. Also, maybe move these functions to where isPromotable is defined instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do. not the most urgent thing right now.
once again, this is a draft.

@pzinn pzinn force-pushed the promote branch 2 times, most recently from be69753 to da0a5fe Compare July 2, 2025 12:03
@pzinn pzinn force-pushed the promote branch 11 times, most recently from 3fa8cc0 to ef2e0e1 Compare July 4, 2025 07:47
@pzinn pzinn mentioned this pull request Jul 4, 2025
@pzinn pzinn force-pushed the promote branch 3 times, most recently from 621457c to d2ac437 Compare July 4, 2025 14:41
@pzinn pzinn mentioned this pull request Jul 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants