View Issue Details

IDProjectCategoryView StatusLast Update
0001793OpenFOAM[All Projects] Bugpublic2015-08-24 18:36
Reporteruser1190Assigned Tohenry 
PrioritynormalSeveritytrivialReproducibilityalways
Status resolvedResolutionfixed 
PlatformGNU/LinuxOSUbuntuOS Version14.04
Product Version 
Fixed in Version 
Summary0001793: activePressureForceBaffleVelocityFvPatchVectorField seems wrong
DescriptionFrom line 299 we have :

if ((mag(valueDiff) > mag(minThresholdValue_) || baffleActivated_))
        {
            openFraction_ =
                max(
                    min(
                        openFraction_
                      + max
                        (
                          this->db().time().deltaT().value()/openingTime_,
                          maxOpenFractionDelta_
                        )*(orientation_),
                        1 - 1e-6
                        ),
                    1e-6
                    );

             baffleActivated_ = true;
        }


I have the feeling it should be :

if ((mag(valueDiff) > mag(minThresholdValue_) || baffleActivated_))
        {
            openFraction_ =
                max(
                    min(
                        openFraction_
                      + min
                        (
                          this->db().time().deltaT().value()/openingTime_,
                          maxOpenFractionDelta_
                        )*(orientation_*sign(valueDiff)),
                        1 - 1e-6
                        ),
                    1e-6
                    );

             baffleActivated_ = true;
        }
TagsNo tags attached.

Activities

wyldckat

2015-08-02 22:30

updater   ~0005169

Having a feeling in science/engineering/mathematics should always be tested, otherwise it's just a feeling.

Can you provide more specific details as to why you believe your proposition is correct?
What is the logic or tests you've done for pointing that consolidated this feeling?

will

2015-08-03 09:22

manager   ~0005170

I think they're saying that, max(/*expression*/, maxValue) is fundamentally faulty logic. It's clipping above the maxValue, not below it. min(/*expression*/, maxValue) is typical. Either that, or the name of the variable is misleading.

I'm not sure where sign(valueDiff) comes from, though. I agree, some proof would be nice, empirical or otherwise.

user1190

2015-08-03 11:31

  ~0005173

The sign(valueDiff) comes from what the bc is supposed to simulate : a check valve.

The valueDiff is the force or pressure difference between the two sides of the patch. Not including it's sign in the calculus of the openfraction would imply the valve to either constantly be opening or closing depending on the chosen value of the orientation.

I've tested a case twice, once with the sign(valueDiff), and once without it.
Without it the valve could only open, and with it the valve was properly oppening and closing.

will

2015-08-03 15:49

manager   ~0005176

Could you upload your case?

user1190

2015-08-04 07:49

  ~0005182

I'm sorry, but i'm not allowed by my company to share any of these cases.

If i find the time, i can make a simplified case though.

alexeym

2015-08-04 08:45

reporter   ~0005184

According to documentation in header file, maxOpenFractionDelta is max open fraction change per timestep. So this

                      + max
                        (
                          this->db().time().deltaT().value()/openingTime_,
                          maxOpenFractionDelta_
                        )*(orientation_),

should be really

                      + min
                        (
                          this->db().time().deltaT().value()/openingTime_,
                          maxOpenFractionDelta_
                        )*(orientation_),

Or in addition to misleading variable name, documentation in header file is wrong.

user21

2015-08-06 21:40

  ~0005203

I agree the code should look like:

        if ((valueDiff > minThresholdValue_) || baffleActivated_)
        {
            openFraction_ =
                max
                (
                    min
                    (
                        openFraction_
                      + min
                        (
                          this->db().time().deltaT().value()/openingTime_,
                          maxOpenFractionDelta_
                        )*(orientation_),
                        1 - 1e-6
                    ),
                    1e-6
                );

             baffleActivated_ = true;
        }

henry

2015-08-18 10:56

manager   ~0005257

With this change is everyone agreed that the implementation is correct and corresponds to the description in the header? If not I propose that this class is removed until we have a specification, implementation and description that are correct and consistent.

alexeym

2015-08-20 20:14

reporter   ~0005278

In fact "if not" branch is quite interesting, since in the report comments everybody was appealing to documentation in the header file, and there were no comments from people, who are actually using the feature.

henry

2015-08-20 20:33

manager   ~0005279

@oim: could you provide some feedback on the correctness/usefulness of this class?

Do the changes described here resolve the issues you were having?

Do you agree with the current documentation for the class?

If not can you propose corrections?

If none of the above I will remove the class.

wyldckat

2015-08-22 22:04

updater  

activePressureForceBaffleVelocityFvPatchVectorField.C (11,976 bytes)

wyldckat

2015-08-22 22:04

updater  

activePressureForceBaffleVelocityFvPatchVectorField.H (8,504 bytes)

wyldckat

2015-08-22 22:10

updater   ~0005281

Last edited: 2015-08-22 22:14

View 2 revisions

I was curious about this and I've taken a better look into this. Specially because I'm concerned that this kind of boundary condition (wall+cyclic hybrid) could be forgotten about if removed.

Here's what I've been able to diagnose:

 - The initial problem here is that this boundary condition was introduced in 2.0.0, without any clear explanation as for what it's really for. This missing information is critical for properly fixing this boundary condition.

 - The closest details available as to why it was introduced, seems to be because it's part of PDRFoam, PDRMesh and the tutorial case "combustion/PDRFoam/flamePropagationWithObstacles".

 - Studying the tutorial case and the description on the header file, it seems that this boundary condition was originally designed to act as a baffle that is "blown away" by a pressure difference above a threshold (i.e. explosion), regardless of being a positive or negative difference.

 - Based on this analysis, this means that this boundary condition was originally _not_ designed to act as a check valve. If anything, it seems to be something like a sheet of metal that gets torn up by the blast, at least within the tutorial.

 - The "orientation" parameter as it is implemented, is used for defining which way the fraction is "opened". In other words, if positive, the "openFraction" increases; if negative, it decreases. Problem here might be that if it's meant to "close" when a high pressure (or force) difference occurs, then this would mean that the baffle could act as a "blast door" that closes when the pressure difference is above the threshold.

 - The "maxOpenFractionDelta" is meant to act as a pseudo-relaxation factor. In other words, if for some reason the simulation deltaT is too large, this acts as a limiter that will restrain the rate at which the "open fraction" is increased/decreased.

 - The downside of simply removing this boundary condition is that the tutorial "combustion/PDRFoam/flamePropagationWithObstacles" needs to be readjusted to not use nor create the baffle, therefore loosing one of the apparent two reasons as to why PDRMesh exists.


Based on this assessment, I've attached the updated 2 files ("activePressureForceBaffleVelocityFvPatchVectorField.[CH]") that aim to fix what seems to be the original intention for this boundary condition, for placing in the folder "src/finiteVolume/fields/fvPatchFields/derived/activePressureForceBaffleVelocity". It uses Alexey's deduction (comment #0005184) and I've updated the description... oops, I only updated the description, and forgot to update the comments for the variables in the "private" section of the class structure... I'll upload the second updated header file in a few minutes.

I also vote that the name of the class and boundary condition is also changed to something more to the point of its existence, something like: triggeredPressureForceBaffleVelocity

wyldckat

2015-08-22 22:15

updater  

activePressureForceBaffleVelocityFvPatchVectorField.H.v2 (8,464 bytes)

wyldckat

2015-08-22 22:20

updater   ~0005282

Attached "activePressureForceBaffleVelocityFvPatchVectorField.H.v2", which has the comment for "Orientation" updated in the private section of the class structure.

I forgot to mention in the previous comment that I've tested this with the tutorial "combustion/PDRFoam/flamePropagationWithObstacles", to double-check if it's working as intended, namely with "orientation = 1". I have not yet tested with "orientation = -1".

In addition, it's still missing a security check for whether the orientation value is 1 or -1... not sure if it's worth implementing such a check.

henry

2015-08-24 18:36

manager   ~0005293

@Bruno: Thanks for collating all the information and updating the files. I am happy to change the name if you are now convinced of the purpose of the BC and that a new name would be more consistent with this purpose.

I am not entirely convinced that the implementation is correct yet but I do not have a specification for this BC otherwise I would probably start again.

Resolved by commit 8685344a0b1ff2c91652121400fd1daaeeeea5ff

Issue History

Date Modified Username Field Change
2015-07-22 14:01 user1190 New Issue
2015-08-02 22:30 wyldckat Note Added: 0005169
2015-08-03 09:22 will Note Added: 0005170
2015-08-03 11:31 user1190 Note Added: 0005173
2015-08-03 15:49 will Note Added: 0005176
2015-08-04 07:49 user1190 Note Added: 0005182
2015-08-04 08:45 alexeym Note Added: 0005184
2015-08-06 21:40 user21 Note Added: 0005203
2015-08-18 10:56 henry Note Added: 0005257
2015-08-20 20:14 alexeym Note Added: 0005278
2015-08-20 20:33 henry Note Added: 0005279
2015-08-22 22:04 wyldckat File Added: activePressureForceBaffleVelocityFvPatchVectorField.C
2015-08-22 22:04 wyldckat File Added: activePressureForceBaffleVelocityFvPatchVectorField.H
2015-08-22 22:10 wyldckat Note Added: 0005281
2015-08-22 22:14 wyldckat Note Edited: 0005281 View Revisions
2015-08-22 22:15 wyldckat File Added: activePressureForceBaffleVelocityFvPatchVectorField.H.v2
2015-08-22 22:20 wyldckat Note Added: 0005282
2015-08-24 18:36 henry Note Added: 0005293
2015-08-24 18:36 henry Status new => resolved
2015-08-24 18:36 henry Resolution open => fixed
2015-08-24 18:36 henry Assigned To => henry