GraphicalModels: multiple uses of `gaussianRing`

[taboege]

Multiple uses of gaussianRing Graph from the GraphicalModels package do not work as expected. The code below constructs rings for two different DAGs, but they turn out to be the same (they should already differ in their generators). It looks like the .graph field in both rings is equal to the first DAG.

This impairs all downstream functionality that requires a gaussianRing constructed from a specific graph, like gaussianVanishingIdeal. Multiple people I talked to, including @bkholler, have noticed this and don't know any workaround.

needsPackage "GraphicalModels";
DAGs = {
  digraph({1,2,3},{{1,2}}),
  digraph({1,2,3},{{1,2},{2,3}})
};
L = DAGs / gaussianRing;
print (L#0).graph;
print (L#1).graph;

It seems that gaussianRing maintains a cache of rings it has constructed before. Perhaps the indexing of that cache does not work as intended?

[d-torrance]

It looks like the rings are cached in a hash table. There's a comment in the code:

M2/M2/Macaulay2/packages/GraphicalModels.m2

Line 689 in 1fd2382

--(kk,s,k,l,p,vv) uniquely identifies gaussianRing in case of MixedGraph input.

Is this correct? kk is the coefficient ring, s, l, p, and k are variable names, and vv is a sorted list of vertices. But shouldn't we also keep track of the directed edges to determine the subscripts of the variables in the ring?

Cc: @lukeamendola, @luisgarciapuente, @roserhp, @olgakuznetsova, @harshitmotwani2015, @sonjapetrovic, @mikestillman

[luisgarciapuente]

This is not correct and one needs to add the directed edges as Doug mentions. Here is an example that creates an issue. R2 does not create a new ring but instead uses R1. This gives the incorrect gaussian vanishing ideal as demonstrated with G3. I personally do not think that this hash table is particularly useful so my suggestion would be to remove this code or fix it by adding the directed edges. Unfortunately, I do not have time to do this change in the near future. G1 = mixedGraph (digraph {{b,{c,d}},{c,{d}}},bigraph {{a,d}}) R1 = gaussianRing G1 gaussianVanishingIdeal R1 G2 = mixedGraph(digraph {{a,{b,c}},{b,{c}},{c,{d}}},bigraph {{a,b},{b,c}}); R2 = gaussianRing G2 gaussianVanishingIdeal R2 G3 = mixedGraph(digraph {{1,{2,3}},{2,{3}},{3,{4}}},bigraph {{1,2},{2,4}}); R3 = gaussianRing G3 gaussianVanishingIdeal R3 Luis

…

On Oct 29, 2024, at 12:54 PM, Doug Torrance ***@***.***> wrote: It looks like the rings are cached in a hash table. There's a comment in the code: https://github.com/Macaulay2/M2/blob/1fd238210e9a37ddaec3b700ccd22f06ff5f28f5/M2/Macaulay2/packages/GraphicalModels.m2#L689 Is this correct? kk is the coefficient ring, s, l, p, and k are variable names, and vv is a sorted list of vertices. But shouldn't we also keep track of the directed edges to determine the subscripts of the variables in the ring? Cc: @lukeamendola <https://github.com/lukeamendola>, @luisgarciapuente <https://github.com/luisgarciapuente>, @roserhp <https://github.com/roserhp>, @olgakuznetsova <https://github.com/olgakuznetsova>, @harshitmotwani2015 <https://github.com/harshitmotwani2015>, @sonjapetrovic <https://github.com/sonjapetrovic>, @mikestillman <https://github.com/mikestillman> — Reply to this email directly, view it on GitHub <#3553 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCQJ3LHHLJJPEHT6M4K3PDZ57KV5AVCNFSM6AAAAABQ2LQWSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBVGA4DOMRSGI>. You are receiving this because you were mentioned.

[sonjapetrovic]

Good points, and I agree with the fix idea. I do not remember why we had a hashable for rings, ask Mike @mikestillman! He made sure we do it that way for some procedural reason. :) On Tue, Oct 29, 2024 at 3:13 PM luisgarciapuente ***@***.***> wrote:

…

This is not correct and one needs to add the directed edges as Doug mentions. Here is an example that creates an issue. R2 does not create a new ring but instead uses R1. This gives the incorrect gaussian vanishing ideal as demonstrated with G3. I personally do not think that this hash table is particularly useful so my suggestion would be to remove this code or fix it by adding the directed edges. Unfortunately, I do not have time to do this change in the near future. G1 = mixedGraph (digraph {{b,{c,d}},{c,{d}}},bigraph {{a,d}}) R1 = gaussianRing G1 gaussianVanishingIdeal R1 G2 = mixedGraph(digraph {{a,{b,c}},{b,{c}},{c,{d}}},bigraph {{a,b},{b,c}}); R2 = gaussianRing G2 gaussianVanishingIdeal R2 G3 = mixedGraph(digraph {{1,{2,3}},{2,{3}},{3,{4}}},bigraph {{1,2},{2,4}}); R3 = gaussianRing G3 gaussianVanishingIdeal R3 Luis > On Oct 29, 2024, at 12:54 PM, Doug Torrance ***@***.***> wrote: > > > It looks like the rings are cached in a hash table. There's a comment in the code: > https://github.com/Macaulay2/M2/blob/1fd238210e9a37ddaec3b700ccd22f06ff5f28f5/M2/Macaulay2/packages/GraphicalModels.m2#L689 > > Is this correct? kk is the coefficient ring, s, l, p, and k are variable names, and vv is a sorted list of vertices. But shouldn't we also keep track of the directed edges to determine the subscripts of the variables in the ring? > > Cc: @lukeamendola <https://github.com/lukeamendola>, @luisgarciapuente < https://github.com/luisgarciapuente>, @roserhp <https://github.com/roserhp>, @olgakuznetsova <https://github.com/olgakuznetsova>, @harshitmotwani2015 < https://github.com/harshitmotwani2015>, @sonjapetrovic < https://github.com/sonjapetrovic>, @mikestillman < https://github.com/mikestillman> > — > Reply to this email directly, view it on GitHub < #3553 (comment)>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/ABCQJ3LHHLJJPEHT6M4K3PDZ57KV5AVCNFSM6AAAAABQ2LQWSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBVGA4DOMRSGI>. > You are receiving this because you were mentioned. > — Reply to this email directly, view it on GitHub <#3553 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABXULJKA3OBZVCWAH4AVI4DZ57UADAVCNFSM6AAAAABQ2LQWSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBVGIZTQNJQGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

--

-- Sonja Petrović Professor, <https://www.sonjapetrovicstats.com/> Applied Mathematics, Illinois Institute of Technology Pronouns: she/her ==== *"We all require and want respect, man or woman, black or white. It’s our basic human right." *-Aretha Franklin

[bkholler]

@lukeamendola and I will be leading a group on graphical models at the upcoming M2 conference in Leipzig. We can make the change Luis suggests or some other fix then.

[lukeamendola]

Good catch! Yes, we will fix it in two weeks during the M2 Workshop in Leipzig!

[taboege]

I ran into this bug for the (n+1)st time yesterday and decided to fix it (see #4106). The documentation of markovRing at least mentions that the resulting ring will be cached, so for gaussianRing I went with the fix that records a sufficient amount of data in the cache key to make it work instead of removing the caching altogether.