fix set Set, add toList Tally #3770

pzinn · 2025-05-02T20:51:15Z

this is another (minor) fix of #3574: the definition of set Set didn't work since set is defined at d level.
Instead we let set be equivalent to set @@ keys on hash tables. We also add a missing toList Tally.

mahrud · 2025-05-02T23:34:18Z

M2/Macaulay2/d/sets.dd

@@ -19,6 +19,7 @@ makeSet(e:Expr):Expr := (
     is v:Sequence do makeSet(v)
     is w:List do if ancestor(w.Class,visibleListClass) then makeSet(w.v)
     else WrongArg("a visible list")
+     is h:HashTable do makeSet(keys(h))


Could you add a typical value comment so methods set also lists (set, HashTable)? Also this should be documented.

Oh, actually, this is worse than set Set := identity because it allocates new memory, no?

yeah I'll rewrite it more cleanly, more like top-level applyValues(h,x->1)

Actually, an easier solution might be to make it so people can install methods on set!

yeah I don't know about that. we've been doing that a lot (superseding the d function, e.g., toList has suffered this fate) but I don't think it's great, it's not very clean. I'd rather basic functions like set be at d level.

what's not clean is having a d function, then renaming it at the m2 level, then introducing a new m2 function which shares the same name as the original d function but which potentially behaves differently. it's not good practice and can make debugging confusing.

Another common approach is to give the d function a slightly different name, often appending "0" to the name. So set could be the method function, and some of the installed methods could be the compiled function set0.

Suppose you could declare methods in the interpreter and directly install new method functions on them in the top level. Would this be confusing? I agree that renaming the function and assigning a method is kind of hacky (and the set0 trick is not much better), but the solution to that is to make it possible to declare methods in the interpreter, not prevent set from becoming a method in the top level!

Suppose you could declare methods in the interpreter and directly install new method functions on them in the top level.

yeah that'd be nice... but unless I'm mistaken this isn't currently possible. I was trying to keep this PR to a minimum.

Yes, it's not possible. My point was that what you called unclean is a kludge until we have this feature, not that you should implement it now :)

Anyway, I think I'd like set to become a method, either in this PR or another time.

mahrud · 2025-05-02T23:35:23Z

M2/Macaulay2/m2/set.m2

@@ -15,7 +15,7 @@ tally String      :=
 tally VisibleList := Tally => tally

 elements = method()
-elements Tally := x -> splice apply(pairs x, (k,v) -> v:k)
+elements Tally := toList Tally := x -> splice apply(pairs x, (k,v) -> v:k)


Could you document (toList, Tally) (perhaps just adding it to the documentation of (elements, Tally)?

decided to add it to toList... but why does the doc say that the input can be an integer??

A, a set, a string, a basic list, *or an integer*, or an instance of a class with the iterator method installed

pzinn · 2025-05-06T07:35:02Z

I think Set entries be sorted when printed, just like HashTable -- I really don't like

i1 : set {a,b,c}

o1 = set {c, a, b}

o1 : Set

mahrud · 2025-05-06T16:21:56Z

M2/Macaulay2/m2/expressions.m2

@@ -1240,7 +1240,7 @@ texMath MapExpression := x -> texMath x#0 | "\\," | (if #x>2 then "\\xleftarrow{
 expressionValue MapExpression := x -> map toSequence apply(x,expressionValue)

 -- moved from set.m2 because of loadsequence order
-expression Set := x -> Adjacent {set, expression keys x}
+expression Set := x -> Adjacent {set, expression sortByName keys x}


I don't think you want to sort for two reasons:

it's slow, for no reason other than aesthetics.

it can be misleading if the order of keys S and net S are different.

then why do we do it for HashTable?

We probably shouldn't, it's misleading.

so should I just revert the sorting for Set, or also remove the sorting for HashTable?

I think just remove it here for now, but separately we should discuss whether both should be sorted or unsorted.

n-m-g · 2025-05-21T16:38:01Z

I don't know too much about the background on this specific topic but I agree that it shouldn't matter if hashTable keys are sorted or not. (At least all checks seem to have passed so it is probably OK.) However, a situation where this might be used (and which might cause bugs that are difficult to detect) is the case of a hash table with integer keys. One would need to look into that further and some methods may implicitly assume the keys to be sorted. A related topic concerns the "spots" function (which I learned from Dan Grayson's ChainComplex code many years ago) and which I think should be part of the core. But this "spots" function and its need has caused some controversy. The aim of the sports function is to output the sorted integer keys (if any) of a hashTable.

mahrud · 2025-05-21T16:57:58Z

M2/submodules/googletest

Submodules were inadvertently changed.

how can I revert the submodule change? this is driving me crazy.

The following should get you back to where it's supposed to be (possibly replacing origin with whatever remote you use for the main M2 repo):

git checkout origin/development -- M2/submodules/googletest

pzinn · 2025-06-04T07:05:41Z

Rereading the changes in this PR, was toList ZZ supposed to be like

toList ZZ := n -> toList(0..n-1)

that would be pretty useful

mahrud · 2025-06-04T14:07:40Z

Rereading the changes in this PR, was toList ZZ supposed to be like
toList ZZ := n -> toList(0..n-1)
that would be pretty useful

I would be worried about people using apply(toList N, ...) instead of just apply(N, ...).

fix set Set, add toList Tally

c5b9d88

mahrud reviewed May 2, 2025

View reviewed changes

mahrud reviewed May 6, 2025

View reviewed changes

d-torrance added under discussion Core Issues involving the Core scripts. Interpreter labels May 8, 2025

mahrud mentioned this pull request May 11, 2025

Making sets Sets #2467

Open

mahrud reviewed May 21, 2025

View reviewed changes

pzinn force-pushed the main branch from 90908c6 to 0db6933 Compare May 22, 2025 12:06

pzinn added 3 commits June 4, 2025 09:03

document toList Tally, set HashTable

ad288bb

improve set HashTable

14de338

fix set error message

0f0802d

pzinn force-pushed the main branch from 54a66a2 to 0f0802d Compare June 4, 2025 07:03

fix set Set, add toList Tally #3770

Are you sure you want to change the base?

fix set Set, add toList Tally #3770

Uh oh!

Conversation

pzinn commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mahrud May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pzinn May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pzinn commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n-m-g commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pzinn commented Jun 4, 2025

Uh oh!

mahrud commented Jun 4, 2025

Uh oh!

Uh oh!

pzinn commented May 2, 2025 •

edited

Loading

mahrud May 2, 2025 •

edited

Loading

pzinn May 5, 2025 •

edited

Loading

pzinn commented May 6, 2025 •

edited

Loading