-
-
Notifications
You must be signed in to change notification settings - Fork 101
Add coupling fields used for CFD-DEM #380
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
base: develop
Are you sure you want to change the base?
Conversation
| IOobject::NO_WRITE), | ||
| fvc::grad(*p_)) | ||
| { | ||
| dataType_ = scalar; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is critical, but necessary. In previous versions, gradP was a scalar for some reason. Maybe we should introduce a normal gradient separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to our versioning scheme, this would be a breaking change / major version jump. But I also see the point that this is wrongly defined (or with a very restrictive assumption).
I also stumbled upon this in another context, and we should indeed make a separate normal gradient. We also have other gradients that need reconsideration or renaming (AlphaGradient, TemperatureGradient, VelocityGradient).
I would suggest that, as a first step, we rename this to PressureGradientVector (or a better name), so that it can be merged as-is and released in a v1.4.0, and then we revamp the fields in a v2 of the adapter.
MakisH
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many nice new features, thanks! Remember to also update the documentation.
I have not looked into details yet. Is there any test case already available?
Note that one of the major updates we were planning with @vidulejs for v2 was the generic readers/writers (#138, #279, #295, #346), and this PR gives us more examples to consider.
| IOobject::NO_WRITE), | ||
| fvc::grad(*p_)) | ||
| { | ||
| dataType_ = scalar; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to our versioning scheme, this would be a breaking change / major version jump. But I also see the point that this is wrongly defined (or with a very restrictive assumption).
I also stumbled upon this in another context, and we should indeed make a separate normal gradient. We also have other gradients that need reconsideration or renaming (AlphaGradient, TemperatureGradient, VelocityGradient).
I would suggest that, as a first step, we rename this to PressureGradientVector (or a better name), so that it can be merged as-is and released in a v1.4.0, and then we revamp the fields in a v2 of the adapter.
Adds