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

Revive contact smoothing #989

Open
wants to merge 28 commits into
base: feature-contacts
Choose a base branch
from

Conversation

matthiasfabry
Copy link
Collaborator

@matthiasfabry matthiasfabry commented Dec 4, 2024

Replacement PR of #953.
Reinstates the code of @gecheline, @aprsa and @kecnry to handle contact binaries exchanging energy at the surface.
I've added the "perfect mixing" case which assumes very efficient mixing in the interior and makes the whole surface barotropic. This case is parameter-free and enables an easy test to be done.

TODO:

  • fix test failures
  • add test coverage
  • review code and remove anything not wanted anymore
  • when squashing & merging, ensure gecheline is listed as co-author

@matthiasfabry matthiasfabry added this to the contact support milestone Dec 4, 2024
@matthiasfabry matthiasfabry requested review from kecnry and aprsa December 4, 2024 19:48
@kecnry
Copy link
Member

kecnry commented Dec 4, 2024

can you please either rebase or merge feature-contacts into this branch to resolve the conflicts in the cached bundles? Hopefully that will then trigger the tests.

…ntact-smoothing

# Conflicts:
#	phoebe/frontend/default_bundles/default_binary.bundle
#	phoebe/frontend/default_bundles/default_contact_binary.bundle
#	phoebe/frontend/default_bundles/default_star.bundle
Copy link
Contributor

@aprsa aprsa left a comment

Choose a reason for hiding this comment

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

It looks good to me, but there is a bit of room still for polish. I understand that this is just the first step to get things working again, but I'd recommend that we use this opportunity and polish the code as well.

phoebe/backend/contacts_smoothing.py Outdated Show resolved Hide resolved
phoebe/backend/contacts_smoothing.py Outdated Show resolved Hide resolved
phoebe/backend/contacts_smoothing.py Outdated Show resolved Hide resolved
phoebe/backend/contacts_smoothing.py Outdated Show resolved Hide resolved
phoebe/backend/contacts_smoothing.py Outdated Show resolved Hide resolved
phoebe/dependencies/distl/distl.py Outdated Show resolved Hide resolved
phoebe/frontend/bundle.py Outdated Show resolved Hide resolved
tests/tests/test_contacts/test_mixing.py Outdated Show resolved Hide resolved
tests/tests/test_contacts/test_mixing.py Show resolved Hide resolved
phoebe/backend/backends.py Outdated Show resolved Hide resolved
@kecnry kecnry requested a review from aprsa January 6, 2025 17:25
Copy link
Contributor

@aprsa aprsa left a comment

Choose a reason for hiding this comment

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

I left a few comments throughout but I believe this is good to go.

b.set_value('mixing_method', 'perfect') # since other mixing methods are arbitrarily parametrized, test this case only
b.set_value('columns', '*') # to populate 'teffs'

assert b.run_checks().passed
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing that, eventually, we'll be able to do better than just test passing checks? It would also be good to find a way to test other mixing methods, but that shouldn't hold up this PR.

b.set_value('teff@secondary', 3000) # so we have different temperatures
b.set_value('mixing_enabled', True)
b.set_value('mixing_method', 'perfect') # since other mixing methods are arbitrarily parametrized, test this case only
b.set_value('columns', '*') # to populate 'teffs'
Copy link
Contributor

Choose a reason for hiding this comment

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

  • is a bit of an overkill; why not just add teffs?

"extern_atmx",
"extern_planckint",
"blackbody",
"ck2004",
Copy link
Contributor

Choose a reason for hiding this comment

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

all the changes to this file are inconsequential; I'd leave the original version.

@@ -144,7 +144,7 @@ null
"kind": "star",
"context": "component",
"description": "Critical (maximum) value of the equivalent radius for the given morphology",
"value": 2.0132751765376384,
"value": 2.013275176537638,
Copy link
Contributor

Choose a reason for hiding this comment

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

all the changes to this file are inconsequential; I'd leave the original version.

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.

4 participants