Skip to content

Api demos #2

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Api demos #2

wants to merge 7 commits into from

Conversation

codejaeger
Copy link
Owner

PR Checklist

If you are contributing to Javis.jl, please make sure you are able to check off each item on this list:

  • Did I update CHANGELOG.md with whatever changes/features I added with this PR?
  • Did I make sure to only change the part of the file where I introduced a new change/feature?
  • Did I cover all corner cases to be close to 100% test coverage (if applicable)?
  • Did I properly add Javis dependencies to the Project.toml + set an upper bound of the dependency (if applicable)?
  • Did I properly add test dependencies to the test directory (if applicable)?
  • Did I check relevant tutorials that may be affected by changes in this PR?
  • Did I clearly articulate why this PR was made the way it was and how it was made?

Link to relevant issue(s)
Closes #
Examples for Graph animation demos

How did you address these issues with this PR? What methods did you use?

Copy link

@Wikunia Wikunia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general it looks like a good first step to me. Maybe you can clarify the small comments I made throughout for now and we can discuss this further on Tuesday.


**Node registration**
```julia
nodes = [Object(@Frames(prev_start()+5, stop=100), GraphNode(i, drawNode; animate_on=:scale, fill_color="yellow", border_color="black", text=string(i), text_valign=:middle, text_halign=:center)) for i in range(1, 8; step=1)]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear how this is linked to ga

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general what does ga do if I need to create the graph nodes and edges myself?

Copy link
Owner Author

@codejaeger codejaeger Jun 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of keeping a reference internally to the currently active graph, like the way there is a CURRENT_VIDEO. This however is not completely free of problems and restricts the way nodes can be added to graphs. I can change it to GraphNode(ga, i, drawNode); ...).

When adding a node, the first important thing it needs to do is generate a new layout. This however, is not done if the mode is static in which case it is generated only once when the whole layout is known.
The second thing, an ordering is kept in the graph object as to which node/edge appeared first. This helps in the utility functions like animate_neighbors where instead of placing actions on all neighboring nodes in the same frame range this ordering can be used to organize them sequentially.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just wondering what ga does with the graph object itself when one has to add the edges and nodes later anyway and what would happen if I add more nodes than in my graph ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yeah I prefer GraphNode(ga, i, drawNode and optionally remove the i when one adds them in order anyway.

```julia
enqueue!(Q, 1)
# The frames argument is optional here.
changeNodeProperty!(ga, 1, :fill_color, "green")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this increase some kind of global frame counter?

Copy link
Owner Author

@codejaeger codejaeger Jun 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that will be the case for now. This will create an action on the node based on the last animation created on the graph through the utility functions.
In the future, when it is possible to specify frames for an action based on an action from a different object, this counter would not be required anymore.

```julia
removeNode!(ag, 2)
addNode!(ag, 7)
addEdge!(ag, 1, 7)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might want to think about other names here to make clear that this has something to do with Javis. Maybe even just Javis.add_edge!

Animating neighboring nodes and edges can be simplified by using the `animate_neighbors` utlity function

```julia
animate_neighbors(ag, 1; animate_node_on=(:fill_color, "green"), animate_edge_on=(:color, "red"))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have something like (fill_color = "green", color = "red") to provide several different things?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I see no problem with that. But the drawing properties specified would be required to be used in the drawing function otherwise animation on that property won't show up.


```julia
# change back the graph to its original form
changeNodeProperty!(ag, (node)->true, :fill_color, "white")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the (node)->true syntax is quite weird. What does that do?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we might be able to also specify several changes like above?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is just a function with a fixed signature applying that property change to all nodes matching the criteria.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe make it the default but that's good to know

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding that you might want to create the function declarations and docstrings next 😉

**Register graph in Javis**
```julia
wg = GraphAnimation(g, false, 300, 300, O;
node_attribute_fn=(g, node, attr) -> n(g, node, attr),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unclear to me what this does

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the type of graph used is not known in this example, to fetch node properties like weight from the input graph this function will be used. These weights are then mapped to a certain drawing property like radius.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering why this doesn't return the normal inf instead of the string as well but that's good to know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants