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

[BUG FIX - ICB] iparam/ipntr sizes may change depending on cases. #247

Merged
merged 2 commits into from
Mar 2, 2020

Conversation

fghoussen
Copy link
Collaborator

And this is a silent bug...

@sylvestre
Copy link
Contributor

Could you please add a test which reproduce the bug and make sure it doesn't happen anymore?
Also please update the changelog
thanks

@coveralls
Copy link

coveralls commented Feb 26, 2020

Pull Request Test Coverage Report for Build 1165

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 74.08%

Totals Coverage Status
Change from base Build 1158: 0.8%
Covered Lines: 16462
Relevant Lines: 22222

💛 - Coveralls

@fghoussen
Copy link
Collaborator Author

Could you please add a test which reproduce the bug and make sure it doesn't happen anymore?

Not really. Always / often happening : allocating more memory than needed (silent-not-hurting bug). The real question is : if 7d341a9 is OK (on LP64), then, could this fix #230 which seems to show up only on ILP64 ? (fix is related to integer )

@fghoussen
Copy link
Collaborator Author

fghoussen commented Feb 26, 2020

All OK... So one can try to make the problem "reappear" here after rebase on #246 to benefit from c170c16 and check out if this could fix what looks like #230 ?

You may not want/need to merge this "bug" (allocating more memory than needed) unless it fixes ILP64 related problems.

@fghoussen
Copy link
Collaborator Author

OK, here : this is a silent-no-hurting bug. This should not create any problem : it's just that you may sometimes allocate a bit more memory than you need (14/11 integers instead of 11/7). You always allocate more : so there should be no worry here.

At first, I suspected this may be the cause of #230 (seems to be ILP64 specific) as it's related to iparam / ipntr which are arrays of integer. Seems my suspicion was wrong...

I let this PR aside for now. No problem if not merged.

@fghoussen
Copy link
Collaborator Author

After looking back at this, this should preferably be merged :

  • I realized that in many cases ipntr size is actually increased in icb*.F90 ! (need more memory than expected)

  • But in icb*.F90 , the sizes are likely muted variables / not-used information (not sure) and are probably "just not checked anyway" (what is checked by compiler is the type, not sure if the size is checked too)

  • In pratical cases, using the same size for iparam / ipntr is more convenient (even when you allocate more than needed) : hunting all cases in the repo to "fix" this would be endless and error-prone... So I don't want to go into this. But maybe you could keep the "fix" in arpackSolver.hpp (as this one is easy : this is the one place where you always pass by)

>> git grep "integer[ ]*iparam" SRC/*[ae]upd*.f
SRC/cnaupd.f:      integer    iparam(11), ipntr(14)
SRC/cneupd.f:      integer    iparam(11), ipntr(14)
SRC/dnaupd.f:      integer    iparam(11), ipntr(14)
SRC/dneupd.f:      integer    iparam(11), ipntr(14)
SRC/dsaupd.f:      integer    iparam(11), ipntr(11)
SRC/dseupd.f:      integer    iparam(7), ipntr(11)
SRC/snaupd.f:      integer    iparam(11), ipntr(14)
SRC/sneupd.f:      integer    iparam(11), ipntr(14)
SRC/ssaupd.f:      integer    iparam(11), ipntr(11)
SRC/sseupd.f:      integer    iparam(7), ipntr(11)
SRC/znaupd.f:      integer    iparam(11), ipntr(14)
SRC/zneupd.f:      integer    iparam(11), ipntr(14)

>> git grep "integer[ ]*iparam" PARPACK/SRC/MPI/*[ae]upd*.f
PARPACK/SRC/MPI/pcnaupd.f:      integer    iparam(11), ipntr(14)
PARPACK/SRC/MPI/pcneupd.f:      integer    iparam(11), ipntr(14)
PARPACK/SRC/MPI/pdnaupd.f:      integer    iparam(11), ipntr(14)
PARPACK/SRC/MPI/pdneupd.f:      integer    iparam(11), ipntr(14)
PARPACK/SRC/MPI/pdsaupd.f:      integer    iparam(11), ipntr(11)
PARPACK/SRC/MPI/pdseupd.f:      integer    iparam(7), ipntr(11)
PARPACK/SRC/MPI/psnaupd.f:      integer    iparam(11), ipntr(14)
PARPACK/SRC/MPI/psneupd.f:      integer    iparam(11), ipntr(14)
PARPACK/SRC/MPI/pssaupd.f:      integer    iparam(11), ipntr(11)
PARPACK/SRC/MPI/psseupd.f:      integer    iparam(7), ipntr(11)
PARPACK/SRC/MPI/pznaupd.f:      integer    iparam(11), ipntr(14)
PARPACK/SRC/MPI/pzneupd.f:      integer    iparam(11), ipntr(14)
@fghoussen
Copy link
Collaborator Author

Rebasing on master as we know now this is no big bug and can't be related to ILP64 problems.

Comment on lines 680 to 682
return 0;
}
else if (aeupd == "eupd") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return 0;
}
else if (aeupd == "eupd") {
return 0;
}
if (aeupd == "eupd") {

The else is useless as we have a return just above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor

Choose a reason for hiding this comment

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

fyi, you could just have press the "commit suggestion" button ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, now I know!

@sylvestre sylvestre merged commit 5c988b0 into opencollab:master Mar 2, 2020
@fghoussen fghoussen deleted the bug2 branch March 5, 2020 12:59
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.

None yet

3 participants