search

hemelb-codes / hemelb

A high performance parallel lattice-Boltzmann code for large scale fluid flow in complex geometries
GNU Lesser General Public License v3.0
34 stars 11 forks source link

Complete modernisation of the collisions / streamers #82

Closed schmie closed 9 years ago

schmie commented 9 years ago

Reported by hywel on 27 Oct 2011 19:23 UTC The Regularised and GuoZhengShi collision / streaming operators need unit tests. The Regularised one may need refactoring as a collision rather than a streamer.

schmie commented 9 years ago

Modified by hywel on 27 Oct 2011 19:23 UTC

schmie commented 9 years ago

Modified by miguel on 15 Dec 2011 13:27 UTC

schmie commented 9 years ago

Comment by miguel on 21 Dec 2011 10:34 UTC I before I get my hands on the code I would like to read a bit about both streamers. I found the article below for Regularised

http://pre.aps.org/abstract/PRE/v77/i5/e056703

But could not find one for !GuoZhengShi. The site below lists plenty of papers with those 3 authors

http://sites.google.com/site/zlguonlcc/

Any idea?

schmie commented 9 years ago

Comment by hywel on 21 Dec 2011 10:50 UTC I'm pretty sure it was described on this Laett & Chopard paper: http://pre.aps.org/abstract/PRE/v77/i5/e056703 but I can't access it from where I am now (outside of UCL).

schmie commented 9 years ago

Comment by miguel on 11 Jan 2012 16:27 UTC Updated regularised BC and unit test added in 6a8766fea0f4.

Hywel, please CR and also comment on the @todo messages in Regularised.h.

schmie commented 9 years ago

Comment by hywel on 11 Jan 2012 16:53 UTC Line 40: I don't think this comment does make sense, like you say. Line 51: I suspect your todo is right, at the least, this code should be made clearer. I wouldn't worry about breaking the unit tests when you're developing this as they're likely to be wrong too. We could consider unit-testing against another implementation of the same BC (Palabos or whatever). Line 54: Yeah, if it's not clear (and possibly wrong), I'd favour coding the calculations clearly without shortcuts and letting the compiler optimise it. Line 125: Yep, it's just f_eq (providing hydroVars.density and hydroVars.v_z, v_y, v_z haven't changed since you constructed hydroVars).

Actually looking at those bits, it might be worth you rewriting this, because it's so hard to follow and near impossible to confirm that it's doing what the paper says it should. What do you think?

At the least, I think we should have the version in the !LbTestsHelper written in a really clear way so we can be certain that the results are right; at the moment, the !LbTestsHelper version looks very similar to the Regularised.h version. The actual test code itself is very clear and well commented though.

So I think we should rewrite the version in !LbTestsHelper to mirror the paper closely, in a way that makes us very confident it's correct. If the tests still pass, we're done, but if they fail we should rewrite the version in Regularised.h. Sound fair?

schmie commented 9 years ago

Comment by miguel on 12 Jan 2012 10:06 UTC Replying to hywel:

Line 40: I don't think this comment does make sense, like you say. Line 51: I suspect your todo is right, at the least, this code should be made clearer. I wouldn't worry about breaking the unit tests when you're developing this as they're likely to be wrong too. We could consider unit-testing against another implementation of the same BC (Palabos or whatever). Line 54: Yeah, if it's not clear (and possibly wrong), I'd favour coding the calculations clearly without shortcuts and letting the compiler optimise it. Line 125: Yep, it's just f_eq (providing hydroVars.density and hydroVars.v_z, v_y, v_z haven't changed since you constructed hydroVars).

Actually looking at those bits, it might be worth you rewriting this, because it's so hard to follow and near impossible to confirm that it's doing what the paper says it should. What do you think?

I agree. Done in [Unit test updated in 1469. Couldn't check regression test as streaklines are playing up again on OS X.

At the least, I think we should have the version in the !LbTestsHelper written in a really clear way so we can be certain that the results are right; at the moment, the !LbTestsHelper version looks very similar to the Regularised.h version. The actual test code itself is very clear and well commented though.

The code in LbTestsHelper is basically the same as in the streamer. I'm not 100% happy with this way of testing things, where we compare against the almost the same implementation. In this cases, I would suggest being a bit more abstract and testing the desired properties of the code, e.g. mass/momentum conservation. It might be more useful in terms of identifying numerical issues.

So I think we should rewrite the version in !LbTestsHelper to mirror the paper closely, in a way that makes us very confident it's correct. If the tests still pass, we're done, but if they fail we should rewrite the version in Regularised.h. Sound fair?

That sounds like a reasonable approach. It does however require a lot of coding on the LbTestsHelper side since the paper does not have the bounce back step that you added and does not define the treatment of corner nodes...

schmie commented 9 years ago

Comment by hywel on 12 Jan 2012 10:33 UTC Much nicer :)

For interest and future coding, instead of "hydroVars.GetFEq().f[you can now write "hydroVars.GetFEq()ii" which is that little bit tidier.

Apologies for the streaklines regression... James is looking into this, with occasional help from me, we should solve it soon.

I like your idea of testing desired / expected properties. It's probably a good form of documentation too.

schmie commented 9 years ago

Comment by hywel on 26 Jan 2012 11:59 UTC We need to get this done before we start using these collisions / streamers for science (but it's not vital for first runs) hence moving to 0.2.1

schmie commented 9 years ago

Comment by hywel on 6 Feb 2012 16:49 UTC See f45c37400a8a

This involves some improved commenting on the GZS streamer class (including two TODOs that we should be aware of).

I've also written a unit test for the streamer which I think is sufficient. Over to Miguel for review.

schmie commented 9 years ago

Comment by miguel on 6 Feb 2012 16:55 UTC Oh man, I've been working on this for the whole day... Please change ticket ownership if working on a ticket assigned to someone else. CR-ing now.

schmie commented 9 years ago

Comment by hywel on 6 Feb 2012 17:00 UTC Sorry Mig, I've been working on it all day today too - had nothing else to do so just scanned tickets for anything standalone for 2.1...

Feel free to throw away my code and use your own. Or take the best of both. Either way, no reason for your work to be for nothing.

schmie commented 9 years ago

Comment by miguel on 6 Feb 2012 18:01 UTC Don't worry, I like your thorough testing of multiple wall configurations better. I've got a couple of suggestions that I discuss below.

Looking good in general, good job documenting the streamer class and the test, just a few comments:

schmie commented 9 years ago

Modified by miguel on 6 Feb 2012 18:05 UTC

schmie commented 9 years ago

Comment by miguel on 6 Feb 2012 18:09 UTC Replying to hywel:

For interest and future coding, instead of "hydroVars.GetFEq().f[you can now write "hydroVars.GetFEq()ii" which is that little bit tidier.

Regularised.h updated in r1636.

schmie commented 9 years ago

Comment by hywel on 7 Feb 2012 13:04 UTC Replying to miguel:

  • Something that took me a while to figure out is how the streaming step in GuoZhengShi.h line 50 worked without a check for neighbour sites outside the domain. I saw that fNew in LatticeData has an extra entry used as a hypothetical neighbour ("rubbish site") by all the links pointing outside the domain. This is a convenient feature, no doubt, but it is poorly documented. Maybe a small description in Site::GetStreamedIndex (or somewhere else...) would help.

Yeah, good point. Done.

  • Re GuoZhengShi.h line 93, I also noticed that. Would it be possible to add an assertion that trips in such case (to save us the debugging). Another option could be to have a richer if condition that sends this case down the else route.

It wouldn't be too hard to write either an "assert" or a test that only runs when the logging level is Debug. They'll both be compiled-out in the normal case. I think using the else route would be a bad idea cos we'd cover the bug.

I've done the log version.

  • Re the same if statement, wouldn't it be less bad to treat the else branch as if delta > 0.75 (i.e. getting rid of the else branch as this is done before)?

Not sure I understand what you mean, sorry.

  • In the test, I'm a bit puzzled by guoZhengShi not being allocated/dellocated and working!

Yeah, that's crazy. I mean, GCC is good for fault-tolerance but that is extreme. Fixed. I also went through and checked (with CPPUNIT_ASSERT(false) lines) that each branch is being hit by the unit test.

Changeset fead3bc77f1d

schmie commented 9 years ago

Comment by hywel on 7 Feb 2012 13:05 UTC Thanks for the update to Regularised btw. All good.

schmie commented 9 years ago

Comment by miguel on 7 Feb 2012 13:40 UTC Replying to hywel:

Replying to miguel:

  • Re the same if statement, wouldn't it be less bad to treat the else branch as if delta > 0.75 (i.e. getting rid of the else branch as this is done before)?

Not sure I understand what you mean, sorry.

As I understood the paper, there are two extrapolation cases: i) u_w = u_w1, when delta>0.75 and ii) u_w = delta*u_w1 + (1-delta)u_w2, otherwise. Using the paper notation, u_w1 is some function of u_f and u_w2 of u_f and u_ff.

If there are boundaries at either side you could in principle evaluate u_f (and therefore u_w1) but not u_ff (nor u_w2).

What I'm suggesting is that in such cases, the extrapolation is done as suggested for delta>0.75, regardless of the distance to the wall.

This is obviously dodgy (in the paper they are concerned with delta being very small and appearing in a denominator in that case), my main concern is that 0VE completely terrifies me!

Whatever you decide to do, the ticket is ready to be closed, IMO.

schmie commented 9 years ago

Comment by hywel on 7 Feb 2012 15:09 UTC I get it now - you're right.

Changed code and unit test for it in d7e11751b282. I'll close the ticket but you're welcome to have a look at the diff anyway.

schmie commented 9 years ago

Comment by miguel on 7 Feb 2012 15:27 UTC Good job! I've learnt a bit about LB BCs on the way...