Skip to content

Mtot as primary, not Msun - #453

Open
awilson110 wants to merge 1 commit into
mainfrom
comet-barycentric
Open

Mtot as primary, not Msun#453
awilson110 wants to merge 1 commit into
mainfrom
comet-barycentric

Conversation

@awilson110

Copy link
Copy Markdown
Collaborator

Fixes #447 .

Replaced the primary to be Mtot instead of Msun

Review Checklist for Source Code Changes

  • Does pip install still work?
  • Have you written a unit test for any new functions?
  • Do all the units tests run successfully?
  • Does Layup run successfully on a test set of input files/databases?
  • Have you used black on the files you have updated to confirm python programming style guide enforcement?

updated the code, the tests and used the linter

I think the linter removed a blank line in test_layup_configs.py
@awilson110
awilson110 requested a review from hannorein July 28, 2026 12:02
@hannorein

Copy link
Copy Markdown
Collaborator

Hi @awilson110, this looks good. I think this should work.

@matthewholman matthewholman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Correct fix for #447rebound.Particle(m=Mtot) is the barycenter at the origin, so the reported elements are now genuinely barycentric (μ = GM_total, position relative to the SSB), and the demo_comet_expected.csv fixtures are regenerated to match.

One thing worth confirming: the pure-REBOUND fallback (include_assist=False, previously sim.particles[0]) now also uses the origin+Mtot primary. That's right as long as simulation_convert_to_rebound leaves the sim in the SSB frame — but that branch only fires for comets past the ASSIST timeframe, so the fixtures probably don't exercise it. A quick check would close it.

Minor: tests/layup/test_layup_configs.py drops one line near the top — intentional, or a stray edit?

@awilson110

Copy link
Copy Markdown
Collaborator Author

Correct fix for #447rebound.Particle(m=Mtot) is the barycenter at the origin, so the reported elements are now genuinely barycentric (μ = GM_total, position relative to the SSB), and the demo_comet_expected.csv fixtures are regenerated to match.

One thing worth confirming: the pure-REBOUND fallback (include_assist=False, previously sim.particles[0]) now also uses the origin+Mtot primary. That's right as long as simulation_convert_to_rebound leaves the sim in the SSB frame — but that branch only fires for comets past the ASSIST timeframe, so the fixtures probably don't exercise it. A quick check would close it.

@hannorein would you be able to confirm that the function simulation_convert_to_rebound keeps the simulation in the barycentric frame? I'd imagine it does, but good to check

Minor: tests/layup/test_layup_configs.py drops one line near the top — intentional, or a stray edit?

That was a mistake, I can remove it if needed

@hannorein

Copy link
Copy Markdown
Collaborator

Yes, simulation_convert_to_rebound does not change the frame.

@matthewholman matthewholman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good

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.

Comet original 1/a_0 should use barycentric elements with GM_total; code uses heliocentric with GM_sun

3 participants