Skip to content

Conversation

@davidscn
Copy link
Member

Adds

  • the drag force
  • explicit and implicit momentum fields
  • writing the pressure gradient

IOobject::NO_WRITE),
fvc::grad(*p_))
{
dataType_ = scalar;
Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

@MakisH MakisH left a 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;
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants