View Issue Details

IDProjectCategoryView StatusLast Update
0001853OpenFOAMBugpublic2015-09-17 22:09
Reporterchriss85 Assigned Tohenry  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
PlatformGNU/LinuxOSOtherOS Version(please specify)
Summary0001853: pairPatchAgglomeration doesn't call compactLevels()
DescriptionI was trying to use pairPatchAgglomeration to create a coarser boundary mesh and I noticed that the class doesn't call compactLevels after agglomeration, to properly resize its lists to the actual count of agglomeration levels. This seems to make it impossible to loop over the levels without knowing the amount of valid levels and is probably not intended.
This function should be called at the end of agglomerate().
TagsNo tags attached.

Activities

chriss85

2015-09-17 11:45

reporter   ~0005363

By the way, OpenFOAM currently doesn't provide any algorithm to create a coarser mesh in a similar fashion to pairPatchAgglomeration?

henry

2015-09-17 12:47

manager   ~0005364

It is not clear what changes you require or what the consequence would be. Have you implemented and tested the change you are asking for?

Currently compactLevels is called as required in

dummyAgglomeration.C
pairGAMGAgglomerate.C
MGridGenGAMGAgglomeration.C

> By the way, OpenFOAM currently doesn't provide any algorithm to create a coarser mesh in a similar fashion to pairPatchAgglomeration

Correct

chriss85

2015-09-17 14:14

reporter   ~0005365

Last edited: 2015-09-17 14:17

Sorry if anything is unclear. I was talking about calling pairPatchAgglomeration::compactLevels() at the end of pairPatchAgglomeration::agglomerate(), in a similar fashion as in the files you mention, so that the patchLevels_ and other lists in the class are correctly sized after patch agglomeration. Please correct me if I have misunderstood something, but in my application it was necessary to add this call to be able to request the number of agglomerated levels from the class after agglomerate() was called. Before adding this call, the list sizes are still at the maximum size and it would've been necessary to check the pointer validity since the list could contain null entries. After calling the function at the end of agglomerate() the lists are correctly sized and it is safe to access their elements.

henry

2015-09-17 16:07

manager   ~0005366

Last edited: 2015-09-17 16:28

Currently the call to compactLevels is made after the call to agglomerate in case other processing is necessary before the levels are compacted. I do not have time right now to work through the implications of moving the call to compactLevels to the end of agglomerate. Have you tested this change? Have you checked through the operation of the code which calls agglomerate and then compactLevels to make sure it is OK to move the compactLevels call into agglomerate?

chriss85

2015-09-17 16:29

reporter   ~0005367

Last edited: 2015-09-17 16:32

The thing is that the compactLevels function is a private one in pairPatchAgglomeration so it cannot be called from elsewhere anyway. The only piece of code I have found that uses the pairPatchAgglomeration class is the faceAgglomerate tool used for viewFactor radiation. This tool doesn't access the patchLevels list in a way that would make the call of compactLevels() necessary, so no official code would be affected by this change.
If the function is called, the unused levels are simply removed from these lists so the result is somewhat more clean. faceAgglomerate only accesses a list that is already initialized with the correct size in agglomerate() directly.

I agree that this isn't a big issue but I initially had some problems here because I was not expecting that the list with the agglomerated patch levels would contain invalid results after agglomeration. A simple call of this function fixes this, so it should be safe to add it.

The only line I have added at the end of pairPatchAgglomeration::agglomerate() is compactLevels(nCreatedLevels);

henry

2015-09-17 17:07

manager   ~0005368

I agree that either compactLevels needs to be called by agglomerate or it has to be public so it can be called at a higher level. I have made this change in OpenFOAM-dev: commit 9f666b0dbd563f7a79855318fabc267f9f70b7f3 and will put it on test.

Issue History

Date Modified Username Field Change
2015-09-17 11:43 chriss85 New Issue
2015-09-17 11:45 chriss85 Note Added: 0005363
2015-09-17 12:47 henry Note Added: 0005364
2015-09-17 14:14 chriss85 Note Added: 0005365
2015-09-17 14:17 chriss85 Note Edited: 0005365
2015-09-17 16:07 henry Note Added: 0005366
2015-09-17 16:28 henry Note Edited: 0005366
2015-09-17 16:29 chriss85 Note Added: 0005367
2015-09-17 16:32 chriss85 Note Edited: 0005367
2015-09-17 17:07 henry Note Added: 0005368
2015-09-17 17:07 henry Status new => resolved
2015-09-17 17:07 henry Resolution open => fixed
2015-09-17 17:07 henry Assigned To => henry