Skip to content

Fix k61r. #6

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

Merged
merged 1 commit into from
Oct 9, 2017
Merged

Fix k61r. #6

merged 1 commit into from
Oct 9, 2017

Conversation

drvinceknight
Copy link
Member

Closes Axelrod-Python/axelrod-fortran#62

See discussion at that issue.

drvinceknight added a commit to Axelrod-Python/axelrod-fortran that referenced this pull request Oct 8, 2017
This requires Axelrod-Python/TourExec#6 to work
but ensures that k61r behaves as it's supposed to.

Closes #62
@@ -2,14 +2,15 @@ FUNCTION K61R(ISPICK,ITURN,K,L,R, JA)
C BY DANNY C. CHAMPION
C TYPED BY JM 3/27/79
k61r=ja ! Added 7/27/93 to report own old value
IF (ITURN .EQ. 1) ICOOP = 0
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a comment that this was a new addition

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah good call.

IF (ITURN .EQ. 1) K61R = 0
IF (ISPICK .EQ. 0) ICOOP = ICOOP + 1
IF (ITURN .LE. 10) RETURN
K61R = ISPICK
IF (ITURN .LE. 25) RETURN
K61R = 0
COPRAT = FLOAT(ICOOP) / FLOAT(ITURN)
IF (ISPICK .EQ. 1 .AND. COPRAT .LT. .6 .AND. R .GT. COPRAT)
z = FLOAT(ICOOP) / FLOAT(ITURN)
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? Seems like it's not...

Copy link
Member Author

Choose a reason for hiding this comment

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

Woops. My mistake, was trying things!

Closes Axelrod-Python/axelrod-fortran#62

See discussion at that issue.

Have written a comment to denote the change.
@meatballs
Copy link
Member

My best guess as what's going on here:

In the documentation for the orginal code , there is a line which says

a. the -SAVEALL option must be used in the Language Systems Fortran compiler to make the local variables in the rule functions static (ie remembers from one call to the next).

By default, when a Fortran function returns, all variables within that function become undefined. The -SAVEALL option would override that behaviour and cause the compiler to save the state of all variables between invocations.

However, that option doesn't exist in the compilers we use today. Instead, we have the -fno-automatic compiler flag for which the documentation says

Treat each program unit (except those marked as RECURSIVE) as if the SAVE statement were specified for every local variable and array referenced in it. Does not affect common blocks. (Some Fortran compilers provide this option under the name -static or -save.) The default, which is -fautomatic, uses the stack for local variables smaller than the value given by -fmax-stack-var-size. Use the option -frecursive to use no static memory.

Our makefile uses the -fno-automatic flag and so the value of ICOOP will definitely be retained between calls to K61R and the behaviour we observe is exactly what you would expect from the code.

Presumably, the original -SAVEALL option behaved differently.

@meatballs meatballs closed this Oct 9, 2017
@meatballs meatballs reopened this Oct 9, 2017
@meatballs meatballs merged commit 091986d into master Oct 9, 2017
@meatballs meatballs deleted the fix-k61r branch October 9, 2017 08:42
@drvinceknight
Copy link
Member Author

Awesome investigation, thanks @meatballs, some of this could end up in the paper 👍

@drvinceknight
Copy link
Member Author

@meatballs are you ok for me to do a new github release for this or would you prefer to do it?

@meatballs
Copy link
Member

Go for it

@meatballs
Copy link
Member

On reflection, I don't think this is to do with -SAVEALL or -fno-automaticat all. Those both affect all invocations of the function and ICOOP is correctly maintaining its state within a given match.

What we are missing here, I think, is any knowledge of how the original tournament was coded.

In our Python implementation, we run all the matches for a tournament within the same session and we are seeing that the maintenance of state across those matches is causing a problem.

But what if that's not how the original tournament was run? It's perfectly possible that the matches were run completely independently of one another and this problem would never have arisen. Without the code for the tournament itself, we'll never know.

@drvinceknight
Copy link
Member Author

But what if that's not how the original tournament was run? It's perfectly possible that the matches were run completely independently of one another and this problem would never have arisen. Without the code for the tournament itself, we'll never know.

Sounds sensible to me. 👍

@marcharper
Copy link
Member

Yeah there's still a missing piece here -- we also have to assumed that ICOOP is initialized to zero on first use so it seems like its matches had to be played in separate invocations of the entire program.

Are we sure that no other functions are affected by this bug?

@drvinceknight
Copy link
Member Author

drvinceknight commented Oct 10, 2017

Sure? No. Pretty sure? Yeah :) I ran the check I mentioned on the PRissue and that didn't flag anything. Once we've got the new results (running) we'll know more.

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.

3 participants