-
Notifications
You must be signed in to change notification settings - Fork 48
Remove merge statements with logical arguments to avoid new ifx compiler errors #141
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
Conversation
… are overwritten or modified.
RobertPincus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are the right way to do things! I don't see why we'd merge Boolean values ever - so even if the compiler is at fault these are sensible changes. Love it, thanks.
|
Thanks very much for finding this! As the original author of ICARUS (this is ~25-30 year old code), I don't remember using the merge command myself. (I suppose I could check to see where this came in). |
|
@jshaw35 I would go ahead and merge this when you're ready, since we know the comparison to KGOs is flawed. Or better even, change the CI to use the older KGOs? |
@RobertPincus I can do that, but there are still some small compiler differences that will require updating the KGOs (see new tests, all differences now at the 1e-7 level). New kgos should also wait until #142 and #143 are merged and variable outputs are clear. @alejandrobodas I'm not sure if you want to replace the ifx v007 because they were incorrect to begin with or go to v008. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#140 shows that updating the ifx compiler has introduced non-physical errors into the KGOs. I have traced these errors to the calculation of clear and cloudy subcolumns within MISR and ICARUS. Both routines use a similar merge call to create a boolean mask over subcolumns:
box_cloudy(1:ncol) = merge(.true.,.false.,tau(j,1:ncol) .gt. tauchk)This call dropped true values in the current ifx compiler, reducing cloud fraction and downstream ISCCP variables (and MISR variables). Replacing these merge statements with explicit or implicit loops over the subcolumn corrects the error (see figure below). I did not find any other occurrences of merge calls applied to boolean variables within the COSPv2.0 source code.
I have checked that changes relative to the ifx v006 kgos for all variables are at the roundoff level (atol and rtol < 1e-6). This does not exclude the possibility that other compiler-specific bugs have crept in, but at least there is nothing we can identify right now. The largest remaining differences between v006 and v007 kgos are on the order of 1e-3 in the dbze94 variable.
This correction, and #139 will require kgo changes. Once the #139 behavior is confirmed I can create a separate PR to bring in both at the same time.
I am optimistic that addressing #139 and #140 will resolve the ifx kgo issues in #127. Hopefully we can merge that PR soon so that a new tag can be created and COSP-RTTOV can be officially brought into CESM.