Skip to content

add Tree implementation - #694

Closed
matthiasdiener wants to merge 14 commits into
mainfrom
tree
Closed

add Tree implementation#694
matthiasdiener wants to merge 14 commits into
mainfrom
tree

Conversation

@matthiasdiener

Copy link
Copy Markdown
Contributor

based on #350 / #690

@inducer

Copy link
Copy Markdown
Owner

The authorship information on the commits is incorrect. For the tree, this should show Kaushik as the author, and you as the committer. For the tests, that should show you as the author, i.e. you need a minimum of two commits.

@matthiasdiener
matthiasdienerforce-pushed the tree branch 2 times, most recently from 15a9057 to 4aacaf5CompareOctober 22, 2022 20:34
@matthiasdiener

Copy link
Copy Markdown
ContributorAuthor

The authorship information on the commits is incorrect. For the tree, this should show Kaushik as the author, and you as the committer. For the tests, that should show you as the author, i.e. you need a minimum of two commits.

I think this should be fixed now.

@matthiasdiener

Copy link
Copy Markdown
ContributorAuthor

This is ready for review

Comment threadloopy/schedule/tree.py Outdated
@@ -0,0 +1,269 @@
__copyright__ = "Copyright (C) 2022 Kaushik Kulkarni"

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.

It's hard for me to see here how this was changed vs. the original.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've split up the commits into Kaushik's original implementation (7f50179), followed by my changes on top of that initial commit. Does that make the changes clearer?

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.

Thanks!

@inducer

Copy link
Copy Markdown
Owner

I will absorb this into #785, closing here.

@inducerinducer closed this Aug 19, 2024
@matthiasdiener
matthiasdiener deleted the tree branch August 20, 2024 18:26
@matthiasdiener

Copy link
Copy Markdown
ContributorAuthor

If you want, you can also cherry-pick the memoization PR we are using in our production version: illinois-ceesd@e886189 (illinois-ceesd#4).


new_child_to_parent = (self._child_to_parent.delete(node)
.set(new_id, parent))
child_to_parent_mut = self._child_to_parent.mutate()

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Because of mutate, this now requires the use of immutables.Map. Could you just use a normal dict for the mutation?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Since the internal immutables.Maps don't seem to be exposed to the user, maybe it is sufficient to just use dict everywhere in this class?

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.

With dict, $O(N)$ modifications of size $O(1)$ of a tree of size $O(N)$ lead to $O(N^2)$ complexity. immutables allows this to remain $O(N)$ (give or take a few $O(\log N)$. So, to my mind, this is the one use case of which I'm aware, where the use of immutables is justified (whereas just about everywhere else, it's just a glorified read-only dict, which is total overkill).

@inducer

Copy link
Copy Markdown
Owner

If you want, you can also cherry-pick the memoization PR we are using in our production version: illinois-ceesd@e886189 (illinois-ceesd#4).

Could you talk about how important those are in terms of cost?

@matthiasdiener

Copy link
Copy Markdown
ContributorAuthor

If you want, you can also cherry-pick the memoization PR we are using in our production version: illinois-ceesd@e886189 (illinois-ceesd#4).

Could you talk about how important those are in terms of cost?

We had seen a ~5% speedup of overall compile time with that PR on a prediction run.

inducer added a commit that referenced this pull request Aug 24, 2024
#694
Co-authored-by: Matthias Diener <mdiener@illinois.edu>
Co-authored-by: Andreas Kloeckner <inform@tiker.net>
inducer added a commit that referenced this pull request Aug 24, 2024
#694
Co-authored-by: Matthias Diener <mdiener@illinois.edu>
Co-authored-by: Andreas Kloeckner <inform@tiker.net>
@inducer

Copy link
Copy Markdown
Owner

I'll add those.

inducer added a commit that referenced this pull request Aug 25, 2024
#694
Co-authored-by: Matthias Diener <mdiener@illinois.edu>
Co-authored-by: Andreas Kloeckner <inform@tiker.net>
inducer added a commit that referenced this pull request Aug 25, 2024
#694
Co-authored-by: Matthias Diener <mdiener@illinois.edu>
Co-authored-by: Andreas Kloeckner <inform@tiker.net>
Sign up for freeto 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.

3 participants

@matthiasdiener@inducer@kaushikcfd