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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mateoatr
Copy link

@mateoatr mateoatr commented May 4, 2018

…rmations in doc

See issue #12

@@ -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
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"

| HigherKinded.Class | `O(s)` | `O(s)` |
| IntAdjacencyMap | `O(log(n))` | `O(s)` |
| NonEmpty | `O(s)` | ` ` |
| Relation | `O(n+m)`| ` ` |
Copy link
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.


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

| removeVertex | Time | Memory |
Copy link
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).

| 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.
Copy link
Owner

@snowleopard snowleopard May 4, 2018

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
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