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.

Sign up for GitHub

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

Jump to bottom

Cartesian: "No Storage" Implementation #868

Merged
merged 44 commits into from
Oct 3, 2022
Merged

Cartesian: "No Storage" Implementation #868

merged 44 commits into from
Oct 3, 2022

Conversation

gronerl
Copy link
Contributor

@gronerl gronerl commented Jul 25, 2022 •
edited
Loading

This PR removes the Storage classes and implements the new "No Storage" concept.

It is currently fully in the cartesian branch but the same shall be applied to functional.

@gronerl
Copy link
Contributor Author

gronerl commented Jul 25, 2022

bors try

bors bot added a commit that referenced this pull request Jul 25, 2022
@bors
Try #868:
34b28f6
@gronerl
Copy link
Contributor Author

gronerl commented Jul 25, 2022

bors try-

@gronerl
Copy link
Contributor Author

gronerl commented Jul 25, 2022

bors try

bors bot added a commit that referenced this pull request Jul 25, 2022
@bors
Try #868:
f19ded4
@bors
Copy link

bors bot commented Jul 25, 2022

try

Build failed:

Linus Groner added 3 commits July 26, 2022 08:35
@gronerl
Copy link
Contributor Author

gronerl commented Jul 26, 2022

bors try

bors bot added a commit that referenced this pull request Jul 26, 2022
@bors
Try #868:
c0d8c4a
@bors
Copy link

bors bot commented Jul 26, 2022

try

Timed out.

@gronerl
Copy link
Contributor Author

gronerl commented Jul 27, 2022

bors try

bors bot added a commit that referenced this pull request Jul 27, 2022
@bors
Try #868:
6b891d0
@gronerl
Copy link
Contributor Author

gronerl commented Jul 27, 2022

bors try-
bors try
😐

bors bot added a commit that referenced this pull request Jul 27, 2022
@bors
Try #868:
7a039dd
@bors
Copy link

bors bot commented Jul 28, 2022

try

Timed out.

@gronerl
Copy link
Contributor Author

gronerl commented Sep 21, 2022

I addressed all comments where I agree and marked as resolved where I deemed it appropriate. We need to discuss making aligned_index optional. Please mark other conversations as resolved if your issue is addressed. Or leave further comments.

@gronerl
Copy link
Contributor Author

gronerl commented Sep 21, 2022

bors try

bors bot added a commit that referenced this pull request Sep 21, 2022
@bors
Try #868:
78fc06f
@gronerl gronerl marked this pull request as ready for review September 21, 2022 12:20
@bors
Copy link

bors bot commented Sep 21, 2022

try

Build failed:

jdahm
jdahm approved these changes Sep 21, 2022
View reviewed changes
Copy link
Contributor

@jdahm jdahm left a comment

Choose a reason for hiding this comment

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

Great work on this!

egparedes
egparedes reviewed Sep 22, 2022
View reviewed changes
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

Just a partial review for now (only the docs part).

docs/gt4py/arrays.rst Outdated Show resolved Hide resolved
docs/gt4py/arrays.rst Outdated Show resolved Hide resolved
docs/gt4py/arrays.rst Outdated Show resolved Hide resolved
docs/gt4py/arrays.rst Outdated Show resolved Hide resolved
docs/gt4py/arrays.rst Outdated Show resolved Hide resolved
docs/gt4py/arrays.rst Outdated Show resolved Hide resolved
docs/gt4py/quickstart.rst Outdated Show resolved Hide resolved
docs/gt4py/quickstart.rst Show resolved Hide resolved
egparedes
egparedes reviewed Sep 23, 2022
View reviewed changes
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

Second part of my review.

src/gt4py/storage/storage.py Show resolved Hide resolved
src/gt4py/storage/storage.py Outdated Show resolved Hide resolved
src/gt4py/storage/storage.py Outdated Show resolved Hide resolved
src/gt4py/storage/storage.py Outdated Show resolved Hide resolved
src/gt4py/storage/storage.py Show resolved Hide resolved
src/gt4py/storage/utils.py Outdated Show resolved Hide resolved
src/gt4py/storage/utils.py Show resolved Hide resolved
gronerl and others added 2 commits September 26, 2022 15:04
@egparedes
Apply suggestions from code review
8ac9142 
Co-authored-by: Enrique G. Paredes <18477+egparedes@users.noreply.github.com>
review comments
b6c3f4c
@gronerl
Copy link
Contributor Author

gronerl commented Sep 26, 2022

I fixed as suggested or left a comment on all your comments. @egparedes

egparedes
egparedes approved these changes Sep 26, 2022
View reviewed changes
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

It looks good.

@gronerl
Copy link
Contributor Author

gronerl commented Sep 29, 2022

bors try

bors bot added a commit that referenced this pull request Sep 29, 2022
@bors
Try #868:
4506a32
@bors
Copy link

bors bot commented Sep 29, 2022

try

Build failed:

@egparedes
Copy link
Contributor

bors try

bors bot added a commit that referenced this pull request Sep 30, 2022
@bors
Try #868:
a431abd
@bors
Copy link

bors bot commented Sep 30, 2022

try

Build failed:

@gronerl
Copy link
Contributor Author

gronerl commented Sep 30, 2022

bors try

bors bot added a commit that referenced this pull request Sep 30, 2022
@bors
Try #868:
41eae34
@bors
Copy link

bors bot commented Sep 30, 2022

try

Build failed:

@egparedes
Copy link
Contributor

bors try

bors bot added a commit that referenced this pull request Oct 3, 2022
@bors
Try #868:
29a3a3c
@bors
Copy link

bors bot commented Oct 3, 2022

try

Build succeeded:

@gronerl gronerl merged commit 786faf4 into GridTools:master Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egparedes egparedes egparedes approved these changes

@jdahm jdahm jdahm approved these changes

@havogt havogt Awaiting requested review from havogt

@tehrengruber tehrengruber Awaiting requested review from tehrengruber

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gronerl @egparedes @jdahm