Skip to content
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

No warning when passing nothing to the immersed kwarg of FieldBoundaryConditions #3961

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

simone-silvestri
Copy link
Collaborator

No description provided.

Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a little dirty, why can't people use DefaultBoundaryCondition? In other words I don't see the point of having two names for this.

@simone-silvestri
Copy link
Collaborator Author

simone-silvestri commented Nov 27, 2024

I thought that nothing is not really a boundary condition, so it kind of seems awkward a warning that says that nothing immersed boundary condition is not supported on a non-immersed grid

A user might think that they need to provide a specific immersed boundary condition, but I am ok closing this without merging.

@glwagner
Copy link
Member

glwagner commented Nov 29, 2024

I thought that nothing is not really a boundary condition, so it kind of seems awkward a warning that says that nothing immersed boundary condition is not supported on a non-immersed grid

A user might think that they need to provide a specific immersed boundary condition, but I am ok closing this without merging.

I guess the default is nothing, right?

The question is, why do users set the bc to nothing? What do we want users to do? Basically the people that suffer from this PR are people trying to read code, because they might be reading code that says something like

bcs = FieldBoundaryConditions(immersed=nothing)

and interpret this as "doing something" (changing a default). But actually, this is not changing the default. So the question is whether we want to encourage people to write the code above or not.

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.

2 participants