search

ESCOMP / CISM

Community Ice Sheet Model
GNU Lesser General Public License v3.0
6 stars 11 forks source link

Fix dummy argument aliasing issue #17

Closed billsacks closed 6 years ago

billsacks commented 6 years ago

NAG version 6.2 catches a problem with passing the same variable to two different arguments of a subroutine, one of which is intent(out). The fix is to use a separate copy of the local variable for one of the arguments.

Fixes #6

billsacks commented 6 years ago

@whlipscomb - can you please review this and let me know if it looks okay? Then I'll need to rebase this to a common ancestor of the master and release branches and merge it into both.

billsacks commented 6 years ago

I have rebased onto the release branch and force pushed. The diffs are the same as before.

billsacks commented 6 years ago

@gunterl okay, you can go ahead and test these changes. I'm going to restart my own testing, too.

gunterl commented 6 years ago

sounds good.

On Fri, Jul 6, 2018 at 4:24 PM, Bill Sacks notifications@github.com wrote:

@gunterl https://github.com/gunterl okay, you can go ahead and test these changes. I'm going to restart my own testing, too.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/cism/pull/17#issuecomment-403161257, or mute the thread https://github.com/notifications/unsubscribe-auth/AH0K-AQDmAca1nPUYywBWTj0BAOYe3MOks5uD-OvgaJpZM4VFp3X .

billsacks commented 6 years ago

I have completed my cism-in-cesm testing with these changes, and everything looks good: The NAG tests pass, and the cheyenne-intel/gnu tests pass and are bit-for-bit.

@gunterl and @whlipscomb once you sign off on these changes I'll bring the changes to master and the release branch. I want to do this in a very specific way to maintain the history I used for my testing in CESM, so please don't merge this yourself.

billsacks commented 6 years ago

Sorry: While I'd like to wait for you two to sign off on this, I'm actually going to go ahead and merge this this weekend: The git steps I need to do are complicated enough that there's a good chance I'm going to mess something up if I wait to do this until I'm back from vacation. (This is complicated by the facts that (1) these fixes are needed ASAP for CESM testing, both on master and the release branch; (2) I'm trying to keep master, release and release-nohist all in sync.)

So you'll probably see this as merged soon (maybe tomorrow). I'll still welcome a review of these changes, though. If you see any problems, we can fix them in a separate commit.

whlipscomb commented 6 years ago

@billsacks, I looked at the changes in parallel_mpi.F90 and glissade_transport.F90, and they seem fine. I also checked out escomp/master (after you merged these changes) and ran some standard standalone tests that (as I expected) had no problems. So I'm ready to sign off. Thanks for taking care of this fix.

gunterl commented 6 years ago

Hi Bill,

I was about to run the stand alone simulation this afternoon but it looks like everything seems to be in working order. Thanks for dealing with this.

Gunter

On Sun, Jul 8, 2018 at 11:25 AM, Bill Sacks notifications@github.com wrote:

Merged #17 https://github.com/ESCOMP/cism/pull/17.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/cism/pull/17#event-1721918462, or mute the thread https://github.com/notifications/unsubscribe-auth/AH0K-OIrBSOd4SZWjyc7sEZ9WhYkQVZGks5uEkCggaJpZM4VFp3X .