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

libHilbert regression #290

Closed
roystgnr opened this issue Jul 10, 2014 · 1 comment
Closed

libHilbert regression #290

roystgnr opened this issue Jul 10, 2014 · 1 comment

Comments

@roystgnr
Copy link
Member

I should have noticed this months ago, but I've been working on application codes that didn't trigger it:

Commit 5588d79 produces a swath of regression test failures in FIN-S

Yes, all that commit does is introduce a single #include line. My suspicion is that we're now getting a different, incompatible overload resolution on Hilbert::coordsToIndex and this is changing the ordering of some of our xdr/xda restart files. I've still no idea of exactly what the changed resolution is doing, nor why this doesn't trigger more failures in FIN-S, nor why it hasn't triggered anything in our examples or other application regression suites.

@roystgnr
Copy link
Member Author

We're definitely getting a different overload resolution: the old code called

coordsToIndex( const CFixBitVec *p, int m, int n, CBigBitVec &h )

but the new code calls

coordsToIndex( const CFixBitVec *p, int m, int n, Hilbert::BitVecType &h )

Hilbert::BitVecType is just a subclass of CBigBitVec with a couple helper methods added, and they look to be interchangeable in most of the code, but setBits has a CBigBitVec specialization which AFAIK won't get called with Hilbert::BitVecType.

This looks like our extension code in hilbert.h. I'm not sure whether "one overloading gives different output from another compatible overloading" qualifies as a bug in libHilbert itself but it's probably worth pinging Chris Hamilton to ask.

Once I've got this straight I'm going to submit a PR that reverts us to the old behavior, both because the libHilbert comments include "This is terribly inefficient" on the default codepath we're hitting now, and because the number of (and importance of) FIN-S regressions created by 5588d79 exceeds the single GRINS regression we'll create by going back to the old codepath.

It's possible that MOOSE is in the same place GRINS is, with affected xdr/xda files generated since the commit. Hopefully the PR will catch that if so.

roystgnr added a commit that referenced this issue Aug 12, 2014
This seems to restore the prior libHilbert behavior (issue #290) and
should be much safer than simply reverting 5588d79 would be.
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

No branches or pull requests

1 participant