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 #2629) Fix NEMOv5 issues #2698

Merged
merged 17 commits into from
Sep 5, 2024
Merged

(closes #2629) Fix NEMOv5 issues #2698

merged 17 commits into from
Sep 5, 2024

Conversation

sergisiso
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (92fbf60) to head (07fec74).
Report is 18 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2698   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files         353      353           
  Lines       48996    49024   +28     
=======================================
+ Hits        48932    48960   +28     
  Misses         64       64           

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

@sergisiso
Copy link
Collaborator Author

@arporter This is ready for review, it compiles NEMOv5 and LFRic with the new spack repository, where there is software stack to build the applications with gfortran-14 nvhpc-24.5 and intel-24.2.

I addition:

  • I updated the version of LFRic, which we now compare using the produced checksum file, so it can be tested with MPI. (note that before we compiled with mpi but run serially which could hide some bugs)
  • I deleted NEMOv5 build system logic to exclude files in order to have them all in the provided script. This allows us to be explicit about what is failing, and fix the problems without having to update the files inside gh_runner manually.
  • I disabled the addition of parallelisation directives inside pure or elemental functions, this produced a compiler bug in NEMOv5 and it seems to be invalid by definition.
  • I fix an issue where we lost the pure/elemental function attributes

I have not yet deleted the current NEMOv5 version that there is in the nemo_test workflow, because the output is very slightly different:
it : 10 |ssh|_max: 0.6483453724882898D+01 |U|_max: 0.5701938594625860D-02 |V|_max: 0.4920016849737715D-01 S_min: 0.2996960233984670D+02 S_max: 0.3102375152368392D+02
it : 10 |ssh|_max: 0.6483453724938688D+01 |U|_max: 0.5701938594606864D-02 |V|_max: 0.4919999106597320D-01 S_min: 0.2996960234052442D+02 S_max: 0.3102375152367826D+02

If the reviewer I ok with this numbers I could update the KGO file, do the nemov5 check for this compiler with exact output match and deleted from the nemov4 workflow.

@sergisiso sergisiso self-assigned this Sep 2, 2024
@sergisiso
Copy link
Collaborator Author

@arporter We are now down to 1 NEMOv4 passthrough error (#717) and 0 NEMOv5 passthrough errors! Although I am wondering if we should enable TOP and ICE at least in the passthrough in order to cover more code.

@sergisiso
Copy link
Collaborator Author

@arporter This is ready for next review

@sergisiso sergisiso changed the title (closes #2692) Fix NEMOv5 issues (closes #2629) Fix NEMOv5 issues Sep 4, 2024
@sergisiso
Copy link
Collaborator Author

I just realised that I made a typo in the branch name and some commits by referring to 2692, but this PR solves issue 2629, sorry for the confusion

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.

Looking good now. I think we 'just' need to get to the bottom of the configuration to use for these tests. As you say, we could include other keys to maximise our exposure but, if that causes problems, we can delay that step.

@sergisiso
Copy link
Collaborator Author

@arporter Ready for another review, I just slightly updated the test filenames and removed del_keys on the workflow.

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.
Integration tests were all green.
Will proceed to merge.

@arporter arporter merged commit e9748d7 into master Sep 5, 2024
13 checks passed
@arporter arporter deleted the 2692_fix_nemo_v5_issues branch September 5, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants