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

refactor[next] Prepare new Field for itir.embedded #1329

Merged
merged 22 commits into from
Sep 5, 2023
Merged

refactor[next] Prepare new Field for itir.embedded #1329

merged 22 commits into from
Sep 5, 2023

Conversation

havogt
Copy link
Contributor

@havogt havogt commented Aug 28, 2023 •
edited
Loading

  • improve TypeAliases
  • add domain and unit_range constructors
  • extract domain slicing utils to next.embedded.common
  • introduce MutableField
  • add some missing operators to Field

samkellerhals
samkellerhals reviewed Aug 28, 2023
View reviewed changes
Copy link
Contributor

@samkellerhals samkellerhals left a comment

Choose a reason for hiding this comment

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

Looks good to me, however I am wondering whether we could pull out some of the method such as _slice so we can reuse them elsewhere

havogt
havogt commented Aug 29, 2023
View reviewed changes
havogt
havogt commented Aug 29, 2023
View reviewed changes
havogt
havogt commented Aug 29, 2023
View reviewed changes
havogt
havogt commented Aug 30, 2023
View reviewed changes
egparedes
egparedes requested changes Aug 30, 2023
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.

The review of the PR has been more complicated than expected because the PR is not small and the changes are not simple. I have many comments, many of them are simple python coding style remarks but there are also some questions and comments about the logic, which I hope I didn't get wrong.

Still need to review the tests in more depth in the next iteration.

@havogt havogt requested a review from egparedes August 31, 2023 15:27
egparedes
egparedes reviewed Sep 4, 2023
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 to me, just minor suggestions. Let me know if you're fully done and I'll approve it

havogt and others added 3 commits September 5, 2023 09:27
@havogt @egparedes
Apply suggestions from code review
9ff35ab 
Co-authored-by: Enrique G. Paredes <18477+egparedes@users.noreply.github.com>
@havogt
address review comments
0ceaaf4
@havogt
fix formatting
d7366cb
egparedes
egparedes approved these changes Sep 5, 2023
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 to me.

@havogt havogt merged commit 8d91a7b into GridTools:main Sep 5, 2023
@havogt havogt deleted the field_refactoring_prep_itir_embedded branch September 5, 2023 09:03
@havogt havogt restored the field_refactoring_prep_itir_embedded branch September 5, 2023 09:03
@havogt havogt deleted the field_refactoring_prep_itir_embedded branch September 5, 2023 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samkellerhals samkellerhals samkellerhals left review comments

@egparedes egparedes egparedes approved these changes

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
@havogt @egparedes @samkellerhals