View Issue Details
ID  Project  Category  View Status  Date Submitted  Last Update 

0003100  OpenFOAM  Bug  public  20181106 05:31  20181112 22:51 
Reporter  albertop  Assigned To  henry  
Priority  normal  Severity  minor  Reproducibility  always 
Status  resolved  Resolution  fixed  
Platform  GNU/Linux  OS  Other  OS Version  (please specify) 
Product Version  dev  
Fixed in Version  dev  
Summary  0003100: Calculation of r in NVDTVD.H  
Description  The value of r used in limiter functions is now calculated as ( https://github.com/OpenFOAM/OpenFOAMdev/blob/master/src/finiteVolume/interpolation/surfaceInterpolation/limitedSchemes/LimitedScheme/NVDTVD.H ): r = 2*(gradcf/gradf)  1 However, in some cases, this may not guarantee positivity of r, so it would be best to take the maximum of this quantity and 0: r = max(0.0, 2*(gradcf/gradf)  1) I am also not sure about the rationale of using: r = 2*1000*sign(gradcf)*sign(gradf)  1 when mag(gradcf) >= 1000*mag(gradf). Wouldn't it be sufficient to ensure that the denominator gradf is finite (>= small number)?  
Tags  No tags attached.  

Similar considerations apply to the "r" function in NVDVTVDV.H 

This form of r was developed and tuned over a very long time, in particular the r = 2*1000*sign(gradcf)*sign(gradf)  1 to avoid division overflow problems. Can you provide an alternative comparison approach which avoids the division which you are happy with? Could you provide any examples for which the current formulation for r is inappropriate? Note that r does not assume that the scheme will be TVD formal so it is not clear that r must be positive; it is the responsibility of the particular scheme to ensure this. 

OK. I did not understand the logic behind r = 2*1000*sign(gradcf)*sign(gradf)  1 because it returns either 2001 or 1999, depending on the sign of sign(gradcf)*sign(gradf). I was thinking about clipping (or summing/subtracting a small value depending on the sign) the denominator when calculating gradcf/gradf, but I have not compared to the current implementation. About the calculations of r, I noticed this because I amddoing a code review for some work I am finishing. I agree most schemes take care of forcing the limiter function to be positive, but not all. For example, to mention one available in OpenFOAM, the symmetric van Albada and the OSPRE schemes have a (not strictly) positive limiter function for r <= 1 and r >= 0, so if r becomes slightly negative, the limiter function is not positive or zero. 

I do not see any problem with the current logic and it replaced a simpler version which added or subtracted a small amount but on some systems with some compilers this caused problems. If you can demonstrate an issue with the current implementation and an alternative with sufficient testing we can consider changing it. It is not clear from your request if you require r to be >= 0 or >= 1 and I have not seen any issues with the operation of van Albada or OSPRE; do you believe either of these schemes needs to be changed? 

In the two schemes mentioned, which are TVD, I think the value of r passed to the limiter function should be positive so that also the limiter function is guaranteed to be positive. In most other limiters this is not necessary because the value of the limiter function is clipped to zero in any case. 

Unless we write separate "r" functions for each scheme this is not possible and if the scheme requires r to be limited in some manner it should do it. This avoids a lot of code duplication by keeping "r" as general as possible and different schemes either don't need to limit "r" or can do what is needed to it. 

I am not asking to write separate r functions. Clipping the r value to zero in the limiter function would be fine. I am still not clear about what scheme would admit r < 0 (none of the TVD), but if it's needed anywhere, that's OK. 

You are correct that TVD schemes would require r to be positive but the current structure supports nonTVD schemes. OK, do you know which limiter functions should be changed? 

I will finish checking the schemes implemented in OpenFOAM and share the proposed changes. 

I have checked all the schemes in the limitedSchemes directory. I think only the van Albada and OSPRE limited schemes may be affected by this, so I propose to use max(0, r) in these two. I attach the two modified files with the proposed change. OSPRE.H (2,795 bytes) vanAlbada.H (2,797 bytes) 

Thanks Alberto, Resolved by commit c3018b3f0779e68aa3e517b0d18ee13ceb9b50a3 Note that I could not commit the patches provided because they were out of date with respect to current OpenFOAMdev, please pull the latest to ensure future patches are consistent. 
Date Modified  Username  Field  Change 

20181106 05:31  albertop  New Issue  
20181106 05:46  albertop  Note Added: 0010134  
20181106 07:17  henry  Note Added: 0010135  
20181106 08:29  albertop  Note Added: 0010137  
20181106 08:58  henry  Note Added: 0010138  
20181106 22:40  albertop  Note Added: 0010145  
20181106 23:01  henry  Note Added: 0010146  
20181106 23:19  albertop  Note Added: 0010147  
20181106 23:37  henry  Note Added: 0010148  
20181107 15:47  albertop  Note Added: 0010154  
20181111 04:53  albertop  File Added: OSPRE.H  
20181111 04:53  albertop  File Added: vanAlbada.H  
20181111 04:53  albertop  Note Added: 0010173  
20181112 22:51  henry  Assigned To  => henry 
20181112 22:51  henry  Status  new => resolved 
20181112 22:51  henry  Resolution  open => fixed 
20181112 22:51  henry  Fixed in Version  => dev 
20181112 22:51  henry  Note Added: 0010177 