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

expose coupled gradient computation #914

Closed
wants to merge 2 commits into from

Conversation

anderson2981
Copy link
Contributor

This encapsulates the part that updates the boundaries and computes the gradients for the coupled system. Basically everything that gets done before calling ns_operator and diffusion_operator. This is required when the driver needs the gradients to compute some other mesh properties, e.g. artificial viscosity.

Updates coupled_ns_heat_operator to use the new implementation.

Questions for the review:

  • Is the scope and purpose of the PR clear?
    • The PR should have a description.
    • The PR should have a guide if needed (e.g., an ordering).
  • Is every top-level method and class documented? Are things that should be documented actually so?
  • Is the interface understandable? (I.e. can someone figure out what stuff does?) Is it well-defined?
  • Does the implementation do what the docstring claims?
  • Is everything that is implemented covered by tests?
  • Do you see any immediate risks or performance disadvantages with the design? Example: what do interface normals attach to?

Copy link
Collaborator

@majosm majosm left a comment

Choose a reason for hiding this comment

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

I wonder if we'd be better off just switching over to the driver-defined coupling as implemented in #902? I think @tulioricci is already using it.

@anderson2981
Copy link
Contributor Author

I wonder if we'd be better off just switching over to the driver-defined coupling as implemented in #902? I think @tulioricci is already using it.

It's certainly possible. I wasn't aware of the other development, so I just tried to rework what was here to support what I wanted to do. At the end of what I did, I don't actually need coupled_ns_operator anymore, I can just directly call the ns_operator and diffusion_operator directly from the driver, but I wanted to preserve other drivers that were using it and have them use the new api.

@majosm
Copy link
Collaborator

majosm commented Jul 7, 2023

Is there anything in this PR that we still want to add? Or did #902 mostly cover it?

@anderson2981
Copy link
Contributor Author

no longer needed.

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