Graphviz issue 233

[chbe-helix]

GraphViz Beta Update Pull Request.

Update includes all code developed to visualize the core factor graph data structure of GraphPPL. This PR resolved issue 233.

We need feedback on the version number and implementation before this is merged. During this time we will run a few additional tests to assure compatibility with GraphPPL v4.3.3.

[albertpod]

Awesome @chbe-helix and @FraserP117! Thanks! We will start testing after IWAI!

[bvdmitri]

Amazing work, also will have more time after IWAI

[wouterwln]

I reviewed some of the code. There seems to be something wrong with the extension loading; I cannot access the structures from the extension when I load GPPL. I'll push a commit refactoring the code a bit, but let's discuss @bvdmitri and @FraserP117 how we can elegantly solve this. We should be able to come up with smth now that we're all in Oxford anyway. Dummy visualizations I was able to cook up look 1000 times nicer than what the current viz looks like though! Thanks guys!

Also on a side note, do you mind renaming the file/module to GraphPPLGraphVizExt? This way, should Julia ever change their extension functionality and expose them, it's clear that this is an extension and to what package it belongs.

Once again, thanks for the work guys!

[FraserP117]

No problem at all, let’s work on this some time while we’re all here. Yes, rename it to whatever you want, I’m easy and I think I speak for Chris and Kobus here also. Thanks!

…

On Mon, 9 Sep 2024 at 11:47 AM, Wouter Nuijten ***@***.***> wrote: I reviewed some of the code. There seems to be something wrong with the extension loading; I cannot access the structures from the extension when I load GPPL. I'll push a commit refactoring the code a bit, but let's discuss @bvdmitri <https://github.com/bvdmitri> and @FraserP117 <https://github.com/FraserP117> how we can elegantly solve this. We should be able to come up with smth now that we're all in Oxford anyway. Dummy visualizations I was able to cook up look 1000 times nicer than what the current viz looks like though! Thanks guys! Also on a side note, do you mind renaming the file/module to GraphPPLGraphVizExt? This way, should Julia ever change their extension functionality and expose them, it's clear that this is an extension and to what package it belongs. Once again, thanks for the work guys! — Reply to this email directly, view it on GitHub <#251 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKMKRHMYYRQXJ7L753FMV2TZVV4DDAVCNFSM6AAAAABNXWRDFKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZXG43TMNZVGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[wouterwln]

This type should be available when we load GraphPPL and GraphViz, I don't really see a quick fix for this but I'm sure we can find something out

[wouterwln]

BTW I really like this strategy type dispatch so I really want to keep this, great work

[bvdmitri]

I like it too, but since we cannot access them from the extension maybe just use Symbols instead, e.g. :simple_iteration and :bfs_traversal (which can be converted to their corresponding structures if necessary and yes it will be type-unstable but this is not performance critical code and the type-instability will be negligible )

[bvdmitri]

e.g. something similar to

convert_symbol_strategy_to_struct(::Val{:simple_iteration}) = SimpleIteration()
convert_symbol_strategy_to_struct(::Val{:bfs_traversal}) = BFSTraversal()

function plot(; strategy = :simple_iteration)
    strategy_struct = convert_symbol_strategy_to_struct(Val(strategy))
    ...
end

I'm think Plots use similar design too

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.61%. Comparing base (c97718a) to head (e49dcb5).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #251   +/-   ##
=======================================
  Coverage   90.61%   90.61%           
=======================================
  Files          15       15           
  Lines        2121     2121           
=======================================
  Hits         1922     1922           
  Misses        199      199           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[chbe-helix]

I reviewed some of the code. There seems to be something wrong with the extension loading; I cannot access the structures from the extension when I load GPPL. I'll push a commit refactoring the code a bit, but let's discuss @bvdmitri and @FraserP117 how we can elegantly solve this. We should be able to come up with smth now that we're all in Oxford anyway. Dummy visualizations I was able to cook up look 1000 times nicer than what the current viz looks like though! Thanks guys!

Also on a side note, do you mind renaming the file/module to GraphPPLGraphVizExt? This way, should Julia ever change their extension functionality and expose them, it's clear that this is an extension and to what package it belongs.

Once again, thanks for the work guys!

Sure! I'll make the update now

[bvdmitri]

There seems to be something wrong with the extension loading; I cannot access the structures from the extension when I load GPPL.

Yes, because it is not possible by design of the extensions. Newly defined structures are local to the extensions and cannot be accessed from outside (without nasty hacks that can be broken in the next versions of Julia). Extensions can only add methods to the existing names using existing names but cannot define new names for the outside world.

This type should be available when we load GraphPPL and GraphViz, I don't really see a quick fix for this but I'm sure we can find something out

maybe something like plot(..., strategy = :simple) and plot(..., strategy = :bfs). Symbol can be converted to TraversalStrategy and const-propagation should make it type-stable and even if its not its shouldn't be a big issue since this code is not performance critical and most of the time will be spent in the plotting anyway.

[FraserP117]

Interesting. Thanks @bvdmitri and @wouterwln. @chbe-helix, Kobus and I will be meeting this week to discuss/address this. Apologies for the delay on my part.

I think @bvdmitri's suggestion of a "Symbol" substitution is probably best for now. I do like the type dispatching and I'd like to have it in the long run. On that, it seems - from your comments - that these types would have to be defined in base GraphPPL.jl (outside of the extension). Is that correct?

I will do some investigating now.