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

(Closes #2854) exclude char assignments from ACC KERNELS regions #2857

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

arporter
Copy link
Member

Small PR that picks up the functionality already in ArrayAssignment2LoopsTrans and copies it into ACCKernelsTrans.

@arporter arporter self-assigned this Jan 17, 2025
@arporter arporter added the NG-ARCH Issues relevant to the GPU parallelisation of LFRic and other models expected to be used in NG-ARCH label Jan 17, 2025
@arporter arporter marked this pull request as draft January 17, 2025 11:47
@arporter
Copy link
Member Author

I'm not entirely happy with the code duplication here but this PR is for OpenACC functionality and the existing check is in the conversion of array assignments to loops so there's not much common ground between them.

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.88%. Comparing base (2307ac7) to head (4c65986).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2857   +/-   ##
=======================================
  Coverage   99.88%   99.88%           
=======================================
  Files         359      359           
  Lines       50833    50847   +14     
=======================================
+ Hits        50777    50791   +14     
  Misses         56       56           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arporter
Copy link
Member Author

I've solved the duplication by making the validation a separate classmethod in ArrayAssignment2LoopsTrans. Coverage is 100%. I'll set the integration tests running.

@arporter
Copy link
Member Author

Integration test for NEMOv4 failed but only because of a small bug where we failed to lower-case the name of a structure component when getting its datatype. I'll re-run that job.

@arporter
Copy link
Member Author

Integration tests now green. Ready for review from anyone :-)

@arporter
Copy link
Member Author

I changed my mind - it turns out my 'fix' revealed general case-sensitivity problems in our storage of components of a StructureType. I've fixed these now and extended the tests slightly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress NG-ARCH Issues relevant to the GPU parallelisation of LFRic and other models expected to be used in NG-ARCH
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant