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

Attempt to simplify/generalize coupling #902

Merged
merged 21 commits into from
Jul 6, 2023

Conversation

majosm
Copy link
Collaborator

@majosm majosm commented May 16, 2023

Deprecates operator definitions in thermal coupling code and refocuses the API on setting up the interface boundaries. This allows for more flexibility on the driver side (e.g., more than 2 volumes, easier access to gradients, etc.).

cc @tulioricci

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?

@majosm majosm marked this pull request as ready for review July 3, 2023 20:23
@majosm majosm requested review from tulioricci and MTCam July 3, 2023 20:24
@majosm
Copy link
Collaborator Author

majosm commented Jul 3, 2023

@tulioricci, @MTCam I think this is ready for a look through. (I'll do the production merge after.)

":func:`add_interface_boundaries` and include them when calling the "
"individual operators instead.", DeprecationWarning, stacklevel=2)

if not use_kappa_weighted_grad_flux_in_fluid:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't we remove it right now and make the warning says that it is being ignored?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 6cd4494.

Copy link
Member

@MTCam MTCam left a comment

Choose a reason for hiding this comment

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

Overall this is great. Only one slight concern from my perspective, but nothing preventing merge I think.

@majosm majosm added the Production Feeder Feeds the production branch label Jul 6, 2023
@majosm majosm merged commit f1602e9 into illinois-ceesd:main Jul 6, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Production Feeder Feeds the production branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants