Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Graph to refer to values, not just addresses #1267

Closed
9 of 10 tasks
rolyp opened this issue Feb 7, 2025 · 7 comments · Fixed by #1281 · May be fixed by #1282
Closed
9 of 10 tasks

Graph to refer to values, not just addresses #1267

rolyp opened this issue Feb 7, 2025 · 7 comments · Fixed by #1281 · May be fixed by #1282

Comments

@rolyp
Copy link
Collaborator

rolyp commented Feb 7, 2025

We should probably change the graph representation so that it “stores” (or rather points to) values rather than just holding their addresses. Currently there is no representation of the “term graph” (the values/expressions themselves construed as graphs, not the DDG) as an explicit map from vertices to “contents”, so there is no way to go back from an arbitrary graph vertex to the actual value at that address.

So although conceptually there is a forest of values constructed during graph evaluation, they “float free” of the graph and aren’t actually reachable from the graph itself. Under the proposed new design, the graph will “cut across” this forest of values. I think this is necessary if we want to navigate to the values associated with intermediate vertices.

Initial thoughts:

  • Edges refer to expressions as well as values. Will we need existentials to store both in the graph?
  • An adjacency map is a Dict that maps vertices (strings) to sets of target vertices. The keys currently have to be strings, so the value/expression associated with that key would thus have to be considered part of the entry (along with the set of target vertices). Maybe we could just store the BaseVal.
  • The new helper for building an edge would have to take the BaseVal as an argument.

See also:

Done/Dropped:

  • Refactor new to take a constructor and the contents to be placed in a VertexData to return the constructed value containing the fresh vertex, and to modify the
  • Appropriately pack base values in primitive operations
  • Newtype wrappers for Dictionary keys, and Matrix indices
    • Vertices' instances for these Newtype‘s
  • Refactor type of HyperEdge from DVertex × Set Vertex to DVertex × Set DVertex
  • Replace Show constraint with a more appropriate one (e.g. TypeName, see below)
  • Rename unDVertex to addresses
  • unPack -> unpack
  • Maybe omit “Backpointers to values” comment – not especially accurate so probably just best left out
  • Remove old vertices implementation
@rolyp rolyp added this to Fluid Feb 7, 2025
@github-project-automation github-project-automation bot moved this to Proposed in Fluid Feb 7, 2025
@rolyp rolyp removed the status in Fluid Feb 7, 2025
@rolyp rolyp moved this to Proposed in Fluid Feb 7, 2025
@rolyp rolyp moved this from Proposed to Planned in Fluid Feb 10, 2025
@JosephBond
Copy link
Collaborator

JosephBond commented Feb 10, 2025

Directly Relevant:

  • Graph
  • Graph.GraphImpl
    • want to retype adjacency maps to be something like AdjMap = Dict (Set Vertex × Exists BaseVal)
    • Using existentials breaks equality for GraphImpl's, restricting to BaseVal Vertex or a Expr Vertex + BaseVal Vertex does not
    • Don't need Monoid/Semigroup on BaseVal, can just initialize to Nothing?
  • Graph.Slice
  • Graph.WithGraph
    • add extra argument to new, and new return type: new :: Set Vertex -> BaseVal -> m (BaseVal Vertex)
  • EvalGraph
  • Primitive.Defs
    Indirectly Relevant:
  • Val
  • App.Fig
  • Test.Util
  • Test.Benchmark.Util
  • Module
  • Pretty

@JosephBond
Copy link
Collaborator

JosephBond commented Feb 10, 2025

Initial Experiment:

type AdjMap = Dict (Set Vertex × ValPointer)
newtype ValPointer = ValPointer (Maybe (BaseVal Vertex))

And pushing "reasonable" additional code through the rest of GraphImpl (setting everything ValPointer related to Nothing) breaks on slicing/array/dims, things just hang.
Edit: issue seems to have been in requiring a Monoid/Semigroup instance for ValPointer. Setting it up to directly push Nothing in as initial values seems to let things work as they should.

I am currently uncertain as to how much benefit we'd get from making this an existential. When a vertex is associated with an expression (as opposed to a value), we can simply have the ValPointer point to Nothing. Perhaps further on in the experiment packing things up as an existential will become paramount.

Add BaseVal argument to new:

Cannot naively add BaseVal argument, since it creates a cycle in module dependencies between WithGraph and Val.
Options:

  • Move ForeignOp and related typedefs to Primitive.Defs: doesn't work, since it creates an indirect cycle between WithGraph, Val, and Primitive.Defs
  • Abstract the BaseVal argument to new, add type parameter to MonadWithGraphAlloc: seems more of an appropriate avenue, but raises more questions:
    • How to handle primitives? Every OpGraph needs to be modified, first to just do nothing with the value
      Thinking that handling stuff at this point is why we might need existentials.
    • Move the idea of the ValPointer into Graph, since both Graph.WithGraph and Graph.GraphImpl inherit from here.

@rolyp rolyp moved this from Planned to In Progress in Fluid Feb 11, 2025
@JosephBond
Copy link
Collaborator

JosephBond commented Feb 13, 2025

I've started pushing the DVertex type through eval. One thing that has come up repeatedly is the following pattern (which I don't really like):

eval _ (Int α n) αs = Val <$> new (insert α (unDVertex αs)) (pack v) <@> v
   where
   v = V.Int n

I'm unsure how to deal with this, right now it feels a little odd to make Val's store DVertex's instead of Vertex's. Then a Val would contain a value and an existential containing that value. The above pattern doesn't appear to slow things down too much in testing, so maybe it's fine to leave as it is for now.

@rolyp do you have any thoughts on whether I should try and figure out how to solve this, or is it worth pushing the Dvertex type through other parts of graph evaluation ?

Edit: performance wise, it doesn't appear to have an impact because Newtype's only exist at compile time, internally they are just tuples. So to resolve this, I'm considering adding a modified insert function which does the unDVertexing, just to clean up the code. Then, the above case will look like this:

eval _ (Int α n) αs = Val <$> new (insert' α αs) (pack v) <@> v
   where
   v = V.Int n

@JosephBond
Copy link
Collaborator

Introducing the new method to the Vertices class may be more problematic than we initially thought, due to the derived instance instance (Functor f, Foldable f) => Vertices (f Vertex). This method is used on EnvExpr, Val Vertex, and ProgCxt. I'm going to try out the naive option and see

@rolyp
Copy link
Collaborator Author

rolyp commented Feb 17, 2025

Try introducing the new method via a new typeclass Vertices', that will avoid the problem.

@rolyp
Copy link
Collaborator Author

rolyp commented Feb 18, 2025

@JosephBond For replacing the Show constraint, I think we probably want our own type class that will define some kind of typeName method (a constant function perhaps returning a type-level string, but let’s just say a string to start with). Then we should be able to write something like:

asVal :: VertexData -> Maybe (Val Vertex)
asVal (VertexData e) = 
  -- use `typeName e` to decide whether `e` contains a value
  -- if so then `unsafeCoerce`  

Then our predicate on vertices will be some other Maybe computation which composes (monadically) with this one.

@rolyp
Copy link
Collaborator Author

rolyp commented Feb 21, 2025

Hi @JosephBond. Before continuing with #1279, let’s get this merged into develop – that will require a careful pass to tidy up any remaining loose ends, remove any spy calls, and also a manual check that the performance of the figure on the landing page of FluidOrg is still good after @jacobpake‘s fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
2 participants