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 #2394) add support char logical in psydata #2509

Merged
merged 24 commits into from
Jan 20, 2025

Conversation

hiker
Copy link
Collaborator

@hiker hiker commented Feb 22, 2024

No description provided.

Copy link

codecov bot commented Feb 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.88%. Comparing base (2307ac7) to head (96f7618).
Report is 25 commits behind head on master.

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

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

@hiker hiker added LFRic Issue relates to the LFRic domain PSyData NG-ARCH Issues relevant to the GPU parallelisation of LFRic and other models expected to be used in NG-ARCH ready for review and removed NG-ARCH Issues relevant to the GPU parallelisation of LFRic and other models expected to be used in NG-ARCH labels Dec 17, 2024
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.

Nice, small PR :-)
Would it be possible to extend one of the examples to make use of/exercise this new functionality? Otherwise it's completely untested :-(
Also, I have a concern about the 'number of bits' being specified for logical and character variables. I can't remember how you use this information?

@hiker
Copy link
Collaborator Author

hiker commented Jan 9, 2025

Nice, small PR :-) Would it be possible to extend one of the examples to make use of/exercise this new functionality? Otherwise it's completely untested :-(

That was a bit of a headache, since neither gocean nor lfric support character strings. So I've added a new generic read-only verification library, and added an example as nemo/eg6. I couldn't add an examples that will actually throw an error (since PSyclone processes the file and does not support loc or sizeof, which was the trick I used in gocean). There is documentation how you can manually modify the psy file to trigger an error (but admittedly that's kind of stupid). Suggestions welcome.

Also, I have a concern about the 'number of bits' being specified for logical and character variables. I can't remember how you use this information?

I have also refactored all of that, so no more bits specified. Unfortunately, all of that that made this PR somewhat bigger :)

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.

Thanks very much Joerg. Just a few minor things now.
In testing the compilation of the examples I discovered that the xdsl Makefile was broken (spaces instead of tabs) so I've taken the liberty of fixing that and pushed it on your branch.

@hiker
Copy link
Collaborator Author

hiker commented Jan 14, 2025

OK, all issues addressed I believe.

@hiker
Copy link
Collaborator Author

hiker commented Jan 14, 2025

Linkspector failures are caused by the new example not yet in git (master). I've started the compilation tests.

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.

All good now thanks Joerg.
I'll proceed to merge.

@arporter arporter changed the title 2394 support char logical in psydata (Closes #2394) add support char logical in psydata Jan 20, 2025
@arporter arporter merged commit f42fe17 into master Jan 20, 2025
11 of 13 checks passed
@arporter arporter deleted the 2394_support_char_logical_in_psydata branch January 20, 2025 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LFRic Issue relates to the LFRic domain PSyData ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants