Skip to content

Conversation

@Whyborn
Copy link
Contributor

@Whyborn Whyborn commented Jan 11, 2026

CABLE

Thank you for submitting a pull request to the CABLE Project.

Description

There were many instances where the "!$" sequence was misused as a regular comment. These lines are treated as code when certain directives are passed e.g. -qopenmp as we do in the ACCESS-AM3 build. There are some lines which are truly OpenMP directives, and these lines are untouched. All other instances of '!$' are changed to be regular comments with '!'.

@JhanSrbinovsky Do you know if this was ever used to contextually include code, through misuse of the -qopenmp option? If this was ever used for practical reasons, those bits should be moved to standard preprocessor directives.

Fixes #670

Type of change

Please delete options that are not relevant.

  • Bug fix

Testing

  • Are the changes bitwise-compatible with the main branch? If working on an optional feature, are the results bitwise-compatible when this feature is off? If yes, copy benchcab output showing successful completion of the bitwise compatibility tests or equivalent results below this line.

📚 Documentation preview 📚: https://cable--671.org.readthedocs.build/en/671/

@JhanSrbinovsky
Copy link
Collaborator

@Whyborn I removed some that were directly interfering. The ones I removed were made by Vanessa Haverd. I hope a test shows this is bitwise comparable and I can confidently avoid having to review every instance over 26 files :)

@Whyborn
Copy link
Contributor Author

Whyborn commented Jan 11, 2026

It will definitely be bitwise compatible in CABLE offline, as we don't set qopenmp in the compilation, and I don't think we do for ESM1.6 either? And given the code isn't integrated directly with AM3, it won't make any difference there either. I'll do a benchcab run just to check, but I can't see how it would possibly fail.

I asked for your review mainly because you may know some of the history as to why this was done in the past. There are some bits that look like they're true comments, some bits that looked like they have been used as debugging by misusing -qopenmp, and some that might have been intended to be included in coupled applications.

@JhanSrbinovsky
Copy link
Collaborator

@Whyborn - You're right - there is no legitimate reason for it. In between CMIPs (so it had no meaning offline as you say) Vanessa added these comments which appeared simply to be her style. There was one instance where some of the !$ lines were a problem (I think it was in canopy), but then this seemed to go away again in another. Anyway, we're better off without them - oh I remember - I think - it was in trying to reproduce ESM1.5 behavior with ESM1.6 (all the since removed cable_runtime%esm15 switches and #IF def ESM15) but I don't think these lines were causing a difference in the output, I think it wasn't even getting that far, maybe the build even. It interpreted it as an OMP directive but then there was nothing valid following it.

Copy link
Collaborator

@JhanSrbinovsky JhanSrbinovsky left a comment

Choose a reason for hiding this comment

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

There were even more of these! I only got rid of the ones that were interfering with whatever it was we were doing at the time (AM3/ESM1.6) From some of the comments I recognize that it was Vanessa Haverd that use "!$" as a comment - very annoying.

I hope that you can do a back to back. test and verify it is bitwise the same so I can confidently avoid reviewing 26 files :)

@Whyborn
Copy link
Contributor Author

Whyborn commented Jan 11, 2026

The benchcab run shows bitwise equivalence. The lines were causing issues when I tried a naive copy of the CABLE science files into JULES, as -qopenmp is specified for the AM3 build.

@Whyborn Whyborn merged commit caeec44 into main Jan 11, 2026
5 checks passed
@Whyborn Whyborn deleted the 670-removed-all-misused-OMP-comments branch January 11, 2026 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Instances of "!$" used for comments

2 participants