Skip to content

promote cleanup#3903

Open
pzinn wants to merge 9 commits intoMacaulay2:developmentfrom
pzinn:promote
Open

promote cleanup#3903
pzinn wants to merge 9 commits intoMacaulay2:developmentfrom
pzinn:promote

Conversation

@pzinn
Copy link
Copy Markdown
Contributor

@pzinn pzinn commented Jul 1, 2025

clean up promote code:

@pzinn pzinn marked this pull request as draft July 1, 2025 14:01
promote(Number,InexactNumber) := (x,RR) -> promote(x,default RR)
promote(ZZ,RR') :=
promote(QQ,RR') :=
promote(RR',RR') :=
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 3 times, most recently from be69753 to da0a5fe Compare July 2, 2025 12:03
@pzinn pzinn force-pushed the promote branch 12 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
@pzinn pzinn marked this pull request as ready for review March 18, 2026 01:20
@d-torrance
Copy link
Copy Markdown
Member

Would you fix the conflict in matrix.m2?

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.

3 participants