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 FortCL initialization options (issue #1019) #1032

Merged
merged 24 commits into from
Jan 15, 2021
Merged

Conversation

sergisiso
Copy link
Collaborator

@sergisiso sergisiso commented Dec 10, 2020

Closes #1019

Adds OpenCL initialization options and updates the OpenCL documentation.

Unfortunately I couldn't see how to do the environment var solution described and preferred in #1019 because in this case the environment var string has to be converted into an integer, probably with the read statement which is not supported in f2pygen, so I went for the configuration file parameter.

It can be tested with the new compile-mpi-ocl in examples/gocean/example3 or in PSycloneBench

@sergisiso sergisiso self-assigned this Dec 10, 2020
@codecov-io
Copy link

codecov-io commented Dec 10, 2020

Codecov Report

Merging #1032 (9ed9046) into master (bfb499c) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1032      +/-   ##
==========================================
- Coverage   98.70%   98.68%   -0.02%     
==========================================
  Files         160      153       -7     
  Lines       31731    31305     -426     
==========================================
- Hits        31319    30893     -426     
  Misses        412      412              
Impacted Files Coverage Δ
src/psyclone/tests/config_test.py 100.00% <ø> (ø)
src/psyclone/tests/psyGen_test.py 100.00% <ø> (ø)
src/psyclone/configuration.py 99.48% <100.00%> (+0.01%) ⬆️
src/psyclone/gocean1p0.py 98.53% <100.00%> (+<0.01%) ⬆️
src/psyclone/psyGen.py 95.76% <100.00%> (+0.04%) ⬆️
src/psyclone/tests/gocean1p0_opencl_test.py 97.68% <100.00%> (+0.26%) ⬆️
src/psyclone/tests/dynamo0p3_builtins_test.py 98.86% <0.00%> (-0.02%) ⬇️
src/psyclone/dynamo0p3.py 99.58% <0.00%> (-0.01%) ⬇️
src/psyclone/tests/dynamo0p3_basis_test.py 98.98% <0.00%> (-0.01%) ⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9423993...e4da20c. Read the comment docs.

@sergisiso sergisiso requested a review from arporter December 21, 2020 13:34
@sergisiso sergisiso added GOcean Issue relates to the GOcean domain ready for review labels Dec 21, 2020
@sergisiso
Copy link
Collaborator Author

@arporter Ready for first review

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Good job Sergi. Just minor comments.
All tests pass, including compilation.
Ditto the examples.
Updated documentation builds fine.
I'll check for pylint/pycodestyle next time.

Please could you update the copyright date in any files you've touched.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Apart from the problem of the offset used in the example kernels, we're down to very minor things now.
All examples are OK, incl. with compilation.
All tests pass, incl. with compilation.
Updated user guide builds fine.
Ref guide builds without new warnings.
I've checked all files for conformance.

re. the kernel-offset issue, I realise I've lost sight of the root cause of this problem. Did you have to change the boundary conditions on the grid in order to solve an execution problem? As this is only an example, I don't think it's worth putting in lots of effort, we just need a simple solution (if there is one).

@sergisiso
Copy link
Collaborator Author

re. the kernel-offset issue, I realise I've lost sight of the root cause of this problem. Did you have to change the boundary conditions on the grid in order to solve an execution problem? As this is only an example, I don't think it's worth putting in lots of effort, we just need a simple solution (if there is one).

@arporter The reason for the changes are limitations on dl_esm_inf distributed memory. Both BC_PERIODIC and the GO_OFFSET_SW stop in the initialization step.

I have now tried to just change to BC_EXTERNAL and comment out the dl_esm_inf lines in the offset_sw initialization:
field_mod.f90:884:

! I think these are correct but we stop because I've not properly
! gone through the coding.
call gocean_stop('cf_sw_init: CHECK non-periodic BCs!')

This already solves the issue with this example, but I don't quite understand what 'gone through the coding' means here? Does it mean that it need proper tests to guarantee that it works?

Ultimately I would like to solve the BC_PERIODIC as well because it is needed for the Shallow benchmark. Which in turn could be used as a test that everything works as expected.

So the options we have is:

  1. Completely change the example to an OFFSET_NE code
  2. Block the MPI Makefile with a TODO to the missing dl_esm_inf functionality (I am ok with this if its temporal as I am already testing it with NemoLite2D)
  3. Make a quick PR that comments out the sw_init non-periodic BC guard
  4. Implement BC_PERIODIC and delete the sw_init guard that then can be tested against the Shallow serial version

I can not assess how safe is 3 or how long would it take to do 4. So I will appreciate your input on this.

@arporter
Copy link
Member

I think option 2. is the best bet with a new PR in dl_esm_inf to develop/test that the necessary functionality works. "By gone through the code" I think I meant I haven't self-reviewed it to check it. If I was a betting man I would bet that it is probably not quite right.

@sergisiso
Copy link
Collaborator Author

@arporter Ready for next review.

@arporter arporter merged commit 29491ed into master Jan 15, 2021
@arporter arporter deleted the 1019_fortcl_options branch January 15, 2021 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GOcean Issue relates to the GOcean domain ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add FortCL initialization options as options in the OCLTrans transformation
3 participants