Macaulay2 / Workshop-2020-Warwick

6 stars 9 forks source link

Naming conflict after copying functions from Graphs #46

Closed olgakuznetsova closed 4 years ago

olgakuznetsova commented 4 years ago

Since we have PackageExports => {"Graphs"} in the preamble of StatGraphs, then the functions defined in Graphs have their precedence. (And we cannot not export or import Graphs because we actually need it inside StatGraphs)

Dan suggested the following workaround: "use the standard synonym for exported symbols: StatGraphs$topologicalSort. Use that everywhere instead of topologicalSort. This will confuse your users, too, since there will be two packages present that define the same symbol, in different ways. But at least they will get a message about shadowing and advice about synonyms."

I tried that and it works. See https://github.com/Macaulay2/Workshop-2020-Warwick/commit/e88c7619ddaeecf093e4ea5af0af688bc85738eb

But that basically means we need to add StatGraphs$ to every line of our code, which doesn't seem practical.

So, I suggest to actually have our own internal version of Graphs where we remove the conflicting functions. @Macaulay2/graphicalmodelsmle what do you think?

harshitmotwani2015 commented 4 years ago

I think it's better to have a totally independent internal version of Graphs (which we call StatsGraph) for our purpose and export this in GraphicalModels and GraphicalModelsMLE.

olgakuznetsova commented 4 years ago

Could you explain you mean by "totally independent"? How does this compare to what we agreed on in issue#38?

harshitmotwani2015 commented 4 years ago

I mean a package which doesn't import/export Graphs

olgakuznetsova commented 4 years ago

Hm, I suggest to wait until others respond in that case. I'm not entirely convinced that we should copy 5k lines of code to work on 100 of them. IMHO, would be easier to have "our" Graphs where we remove everything that's mentioned in Issue #38

to 20. elok. 2020 klo 14.00 harshitmotwani2015 (notifications@github.com) kirjoitti:

I mean a package which doesn't import Graphs

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Macaulay2/Workshop-2020-Warwick/issues/46#issuecomment-677527159, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZFME5KA7Z76DGNXZDPGNTSBT63XANCNFSM4QFJWYDA .

harshitmotwani2015 commented 4 years ago

Yes, I agree.

olgakuznetsova commented 4 years ago

@harshitmotwani2015 I've started the clean up of Graphs on branch OK-GraphsCleanUp

Could you check this commit? If you are ok, I'll make a pull request to the AlgebraicStatistics branch. https://github.com/Macaulay2/Workshop-2020-Warwick/commit/053dd559060d5bf5bd709c0f1e0c8e525375f89c

Also, I added some comments in issue #47

harshitmotwani2015 commented 4 years ago

@olgakuznetsova I think this commit is fine and you can make a pull request to AlgebraicStatistics branch.

olgakuznetsova commented 4 years ago

closed in c8502dccac41cc00df2f59a19a6abf2ea91fcc0a