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

Remove 2D and packing version from gcl #1233

Merged
merged 17 commits into from
Mar 28, 2019
Merged

Remove 2D and packing version from gcl #1233

merged 17 commits into from
Mar 28, 2019

Conversation

mbianco
Copy link
Contributor

@mbianco mbianco commented Mar 22, 2019 •
edited
Loading

This PR removes untested and unused code from GCL, namely the 2D cases (they can be implemented by reducing the third dimension of a 3D case to 1) and the packing version (only the manual is tested and used).

API changes:

  • halo_exchange_dynamic_ut does not take the packing version as last template argument
  • halo_exchange_generic does not take the number of dimensions and the packing versions as template argument

@mbianco
Copy link
Contributor Author

mbianco commented Mar 23, 2019

launch strgrid

@mbianco
Copy link
Contributor Author

mbianco commented Mar 25, 2019

launch strgrid

mbianco
mbianco commented Mar 25, 2019
View reviewed changes
docs_src/manuals/getting_started/code/CMakeLists.txt Outdated
@@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.12.4)

project(GridTools-laplacian LANGUAGES CXX)

find_package(GridTools 0.21.0 REQUIRED)
find_package(GridTools 0.22.0 REQUIRED)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know why this is here...

Copy link
Contributor

Choose a reason for hiding this comment

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

If you merge with master, it should be gone.

@mbianco
Copy link
Contributor Author

mbianco commented Mar 25, 2019

launch strgrid

@mbianco mbianco requested a review from havogt March 25, 2019 12:47
@mbianco mbianco changed the title Remove 2 d from gcl (clean up some unused code in GCL) Remove 2D and packing version from gcl Mar 25, 2019
havogt
havogt approved these changes Mar 25, 2019
View reviewed changes
Copy link
Contributor

@havogt havogt left a comment

Choose a reason for hiding this comment

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

Approved after the changes in comments are in.

include/gridtools/communication/halo_exchange.hpp Outdated Show resolved Hide resolved
include/gridtools/communication/halo_exchange.hpp Outdated Show resolved Hide resolved
@havogt
Copy link
Contributor

havogt commented Mar 25, 2019

COSMO works with these changes.

@mbianco
Copy link
Contributor Author

mbianco commented Mar 25, 2019

I'm waiting for other comments or for merge

lukasm91
lukasm91 requested changes Mar 26, 2019
View reviewed changes
Copy link
Contributor

@lukasm91 lukasm91 left a comment

Choose a reason for hiding this comment

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

Two additional questions / comments:

  • Is HOSTWORKAROUND still needed? That would be nice to remove if not needed (and pretty simple too)
  • Do we need to adapt documentation?

include/gridtools/communication/low_level/Halo_Exchange_2D.hpp Show resolved Hide resolved
include/gridtools/communication/high_level/descriptors_dt.hpp Show resolved Hide resolved
@mbianco
Copy link
Contributor Author

mbianco commented Mar 26, 2019

launch strgrid

@mbianco
Copy link
Contributor Author

mbianco commented Mar 26, 2019

Thanks Lukas, I'll update the documentation. About the workaround, it was to fix GPU related stuff, but we do not test it, so I think it's safer to remove it and introduce it again in case we need it

@mbianco
Copy link
Contributor Author

mbianco commented Mar 26, 2019

launch strgrid

@mbianco
Copy link
Contributor Author

mbianco commented Mar 26, 2019

I fixed Lukas comment, and waiting for the tests to run...

@mbianco mbianco requested a review from lukasm91 March 26, 2019 12:58
@mbianco
Copy link
Contributor Author

mbianco commented Mar 26, 2019

I realized I'm working on eth-cscs and not my fork... I cloned the wrong one because of the policy on daint for deleting files... I'm going to update the documentation (and maybe another small API change)

@mbianco
Copy link
Contributor Author

mbianco commented Mar 26, 2019

launch strgrid

@lukasm91
Copy link
Contributor

launch strgrid

@lukasm91
Copy link
Contributor

I pushed my fix for GCC 8 directly into the PR.

@anstaf
Copy link
Contributor

anstaf commented Mar 27, 2019

launch icgrid

@havogt
Copy link
Contributor

havogt commented Mar 28, 2019

let me run the branch again with COSMO before the merge

@havogt
Copy link
Contributor

havogt commented Mar 28, 2019

works with COSMO

@havogt havogt merged commit 412eba6 into master Mar 28, 2019
@mbianco mbianco deleted the remove_2D_from_GCL branch March 28, 2019 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@havogt havogt havogt approved these changes

@lukasm91 lukasm91 lukasm91 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.
4 participants
@mbianco @havogt @lukasm91 @anstaf