Macaulay2 / Workshop-2020-Warwick

6 stars 9 forks source link

Functions to copy from Graphs to StatsGraph #38

Closed harshitmotwani2015 closed 4 years ago

harshitmotwani2015 commented 4 years ago

1) Graphs Datatype 2) Digraphs datatype 3) Mixed graphs datatype 4) delete edges 5) delete vertices 6) descendants 7) sort vertices 8) parents 9) non neighbours 10) bfs 11) dfs 12) topological sort 13) collate vertices 14) add edge 15) add vertices 16) top sort 17) isConnected 18) isCyclic 19) children

olgakuznetsova commented 4 years ago

@harshitmotwani2015 IMHO, some of these do not need to be copied but instead we need to include "PackageExports=>{"Graphs"}" in the preamble because we want StatGraphs to contain only the functions specific to statistical graphs. I am referring to:

Could you tell where to find "sort vertices". I can't seem to find it in the code for Graphs.m2 or GraphicalModels.m2

olgakuznetsova commented 4 years ago

Or did you mean to specialise those functions to MixedGraphs? (Like: https://github.com/Macaulay2/Workshop-2020-Warwick/blob/f1b48b2dc52bbfa33e1aab6f158e312983933ea6/AlgebraicStatistics/MLE/StatGraphs.m2#L84-L90)

harshitmotwani2015 commented 4 years ago

@olgakuznetsova I was thinking that we don't export graphs package at all and make our stats graph package totally independent from it by having those functions which we will need for graphical models and graphical models mle. And this list contains all the functions used from graphs package in graphical models. But we can also go according to your approach.

And it is sort vertices here https://github.com/Macaulay2/Workshop-2020-Warwick/blob/f1b48b2dc52bbfa33e1aab6f158e312983933ea6/AlgebraicStatistics/MLE/GraphicalModels.m2#L533

It's just a sorting function in M2. Sorry, I confused it with graphs function.

olgakuznetsova commented 4 years ago

@harshitmotwani2015 Thanks for the clarification! Yep, I think it's better to only keep the stuff that is unique to statistical graphs in the StatGraphs package for two reasons:

  1. User might need to use both Graphs and StatGraphs packages in their code. Functions with the same name will create conflicts and break their code.
  2. Graphs team might want to improve the code inside their functions. This will not be available in our package.

So, I think we want the packages to be complementary but not overlapping.

harshitmotwani2015 commented 4 years ago

@olgakuznetsova Thank you very much for clarifications. I will then just keep those functions in stats graph which are highly specific to graphical models. But, I will keep this list as these are all the functions we need for graphical models.

olgakuznetsova commented 4 years ago

Thanks! And we may need to specialise some of the functions (e.g. we need to decide whether a user should be able to use add vertices directly with mixed graphs or perform that outside of this package) El 6 ago 2020 15:52 +0300, harshitmotwani2015 notifications@github.com, escribió:

@olgakuznetsova Thank you very much for clarifications. I will then just keep those functions in stats graph which are highly specific to graphical models. But, I will keep this list as these are all the functions we need for graphical models. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

luisgarciapuente commented 4 years ago

I completely agree with Olga’s suggestions.

Thanks

Luis

On Aug 6, 2020, at 11:07 AM, olgakuznetsova notifications@github.com wrote:

 Thanks! And we may need to specialise some of the functions (e.g. we need to decide whether a user should be able to use add vertices directly with mixed graphs or perform that outside of this package) El 6 ago 2020 15:52 +0300, harshitmotwani2015 notifications@github.com, escribió:

@olgakuznetsova Thank you very much for clarifications. I will then just keep those functions in stats graph which are highly specific to graphical models. But, I will keep this list as these are all the functions we need for graphical models. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

luisgarciapuente commented 4 years ago

I think that the actual list is:

"topSort", "newDigraph", "SortedDigraph", "Bigraph", "bigraph", "LabeledGraph", "labeledGraph", "MixedGraph", "mixedGraph", "collateVertices"

harshitmotwani2015 commented 4 years ago

Thank you very much @luisgarciapuente. I will add these functions to the stats graph package along with the necessary specializations as suggested by @olgakuznetsova.

luisgarciapuente commented 4 years ago

Update on Graphs.m2: I contacted the current list of maintainers and nobody replied. I tried several different email addresses since it seems that some of them have moved to other institutions. I also tried to contact some of the original authors. I only got one reply (from Amelia Taylor) essentially stating that we were free to make changes and add our names to the list of contributors of the package.

olgakuznetsova commented 4 years ago

Thanks, @luisgarciapuente! To me, that sounds like one more good reason to have a separate package :) but it's sad: Graphs is a nice and important package