MacaulayPosets package for JSAG #4056
Conversation
added MacaulayPosets to distributed-packages list
Spelling fixed
|
Converted to a draft so we don't accidentally merge this too soon (which I've done before!) |
d-torrance
left a comment
d-torrance
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I have a number of suggestions for the code.
| "AllOrders" | ||
| } | ||
| needsPackage "Posets"; | ||
| needsPackage "Visualize"; |
There was a problem hiding this comment.
needsPackage "Posets" is redundant since it's already in PackageExports above, and it would be good to add Visualize there instead (or PackageImports, depending on whether you want users to also have access to its functionality or not).
|
|
||
| map(Poset, Poset, HashTable) := PosetMap => opts -> (Q,P,f) -> ( | ||
| -- check that the domain of f is P | ||
| if not set keys f == set vertices P then error "The keys must be vertices of the source Poset."; |
There was a problem hiding this comment.
Error message in M2 are traditionally lower case.
| ); | ||
| map(Poset, Poset, List) := PosetMap => opts -> (Q,P,L) -> ( | ||
| -- Either everything in L a vertex of Q, | ||
| if instance(scan(L, i -> if not isMember(i, vertices Q) then break false), Nothing) then ( |
There was a problem hiding this comment.
This can be simplified to isSubset(L, vertices Q)
| M = super basis(S); | ||
| M = flatten entries M; | ||
| ) else ( -- If the quotient ring doesn't have a finite number of elements: | ||
| M = new MutableList; |
There was a problem hiding this comment.
Do you know what the length of the list will be ahead of time? The way this is currently implemented (calling M#(#M) a bunch), we'll be re-sizing M and creating a brand new list every time.
There was a problem hiding this comment.
And if you don't know the length, then while ... list can be useful. It expands the list as needed, but not at every step. (I think it does something like double the size when it's about to run out of space.)
|
|
||
| getMons( Ideal ) := options -> (I) -> ( -- If the input is an ideal. | ||
| R := ring I; | ||
| if not isPolynomialRing R then return "The given ideal must be an ideal of a polynomial ring."; |
There was a problem hiding this comment.
This should probably be error instead of return
| ) else ( | ||
| flag = false; | ||
| ); | ||
| flag |
There was a problem hiding this comment.
This function can be simplified to one line: #(minimalElements poset) == 1
| ); | ||
|
|
||
| getUniqueMin = (poset) -> ( | ||
| if ( checkUniqueMin(poset) == true ) then ( |
There was a problem hiding this comment.
The == true here is redundant since checkUniqueMin returns a boolean
| if ( checkUniqueMin(poset) == true ) then ( | ||
| el := minimalElements poset | ||
| ); | ||
| el |
There was a problem hiding this comment.
What happens if checkUniqueMin poset returns false? I suppose there should be an error
| return( map(Q, P, new HashTable from apply(#P_*, i->P_*#i=>L#i)) ); | ||
| ); | ||
| -- or everything in L is an Option and not everything in L is a vertex of Q | ||
| if instance(scan(L, i -> if not instance(i, Option) then break false), Nothing) then ( |
There was a problem hiding this comment.
This can be simplified to all(L, i -> instance(i, Option))
| -- For each covering relation k={k#0,k#1} with k#1 in S, the element k#0 is supposed to be in the lower shadow of S. | ||
| for k in coveringRelations(P) do ( | ||
| if any(S, s->s===k#p) then ( | ||
| shadow = append(shadow, k#(p-1)); |
There was a problem hiding this comment.
Using append inside a loop like this isn't efficient. You could do something like:
for k in coveringRelations P list if any(S, s -> s === k#p) then k#(p - 1) else continue2 participants
Introducing package MacaulayPosets which checks properties of posets related to shadow size.