Skip to content

Add overview of functionality/performance for different graph transfo… - #64

Open
mateoatr wants to merge 1 commit into
snowleopard:mainfrom
mateoatr:master
Open

Add overview of functionality/performance for different graph transfo…#64
mateoatr wants to merge 1 commit into
snowleopard:mainfrom
mateoatr:master

Conversation

@mateoatr

@mateoatr mateoatr commented May 4, 2018

Copy link
Copy Markdown

…rmations in doc

See issue #12

Comment thread doc/sketch.md
@@ -0,0 +1,83 @@
This documents intends to give an overview of the time complexity of the different graph transformations implemented by the various data types existent in Alga.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

"time" -> "time and memory"

Comment thread doc/sketch.md
| HigherKinded.Class | `O(s)` | `O(s)` |
| IntAdjacencyMap | `O(log(n))` | `O(s)` |
| NonEmpty | `O(s)` | ` ` |
| Relation | `O(n+m)`| ` ` |

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You didn't cover Graph in this table -- is this because the implementation simply reuses HigherKinded.Class.removeVertex?

I think it would be nice to have an entry for Graph as well, since at some point we might change the implementation and it will no longer rely on HigherKinded.Class.

Comment thread doc/sketch.md

**`removeVertex`**: Removes a vertex from a given graph. In `NonEmpty` graphs, returns `Nothing` if the resulting graph is empty.

| removeVertex | Time | Memory |

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The first header here should probably be Data type instead of removeVertex (same for other tables).

Comment thread doc/sketch.md
| NonEmpty | `O(s)` | `O(s)` |
| Relation | `O(log(m))` | ` ` |

**`replaceVertex`**: Replaces a vertex `x` with a vertex `y` in a given `AdjacencyMap`. If `y` already exists, `x` and `y` will be merged.

@snowleopard snowleopard May 4, 2018

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

"AdjacencyMap" -> "graph".

@snowleopard

Copy link
Copy Markdown
Owner

@mateoatr Many thanks for the PR! I've added a couple of comments above.

Also two general comments:

  • For some reason you do not cover the most important data type Graph -- please add it to the tables.
  • Could you please make sure the lines fit into around 80 characters, so it'll be easier to check diffs in future?

I haven't yet checked the complexity bounds themselves -- will do that soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants