Report the pre-impact eccentricity, and enforce the formatter - #14
Merged
Conversation
Each impact record carries the target's semi-major axis on both sides of the collision but its eccentricity only after. A consumer following a planet on a different orbit from the recorded body can therefore apply the semi-major axis as the fractional change the collision made, which is what PROTEUS does, but has nothing to do for the eccentricity except transplant an absolute value that belongs to this body's orbit rather than to the planet being simulated. That silently overwrites a configured eccentricity, and since tidal heating goes as the square of it, the effect is large and invisible. The merge record now reports e_before alongside a_before, read from the state before the merger overwrites it, so both elements can be applied as changes. That value is the eccentricity at the moment of the collision, which viscous stirring has usually excited well above the value the system started from, so it is the geometry the collision actually happened at.
A formatting sweep with no behaviour change, so the check added next has a clean tree to enforce against and its first run does not bury a real change under hundreds of reformatted lines.
Linting was checked but formatting was not, so style drifted freely: sixteen of the twenty-four source and test files did not match the formatter. The pre-commit hook for it was present but commented out, and CI ran only the linter, which leaves the rule written down in the contributor guide and enforced nowhere. CI now runs the format check alongside the lint, and the pre-commit hook is enabled so it is caught before a push rather than in review. The generated version file is excluded, since the build writes it in its own style into a path that is not tracked, so no contributor could bring it into line and every fresh checkout would fail the check.
Three gaps in the previous two commits. The record's pre-impact eccentricity was pinned only in a unit test on an equal-mass pair, where both bodies carry bit-identical eccentricities and reading the wrong one is invisible. Copying the post-impact value into the field, the mistake this diff most invites, passed the whole suite. There is now a pinned case on a 1.0 and 0.3 Earth-mass pair, where the rejection sampling scales each body's excitation by the square root of the other's mass so the two are distinguishable, and the full-run test asserts both that the field stays in range on every record and that it differs from the post-impact value. Both mistakes now fail. The format gate was scoped to the packaged directories, which left the tracked plotting script at the repository root unchecked, and it was already out of format. It is now formatted and inside the gate, so the enforcement covers every tracked Python file rather than most of them. CI installed the formatter unpinned while the commit hook pinned it, so a future release that shifts the formatter's output would have made the two disagree and turned the hook into a source of failures rather than a way to avoid them. CI now installs the version the hook uses.
The excitation applied to a pair before a collision is drawn without an upper bound, and the fallback taken when no draw satisfies the overlap condition is not bounded either, so a body can reach the merge branch already past e = 1. The ejection branch elsewhere treats exactly that condition as grounds for removing a body, so the two branches disagree about what an unbound orbit means. Over twenty thousand randomised crossings, five of 8268 merge records came out describing an open orbit, three of them through the post-impact value alone, which has been reported that way all along. Everything a consumer derives from these records assumes a closed orbit: periapsis, the time-averaged separation, the Hill radius. The coupled framework refuses an entire impact history rather than use one that is not closed, so a single such record loses a whole run. Both eccentricities are now capped where the record is written, and a capped record warns, naming the body and which side of the collision it applied to. The model's own state is untouched: the cap makes the record usable, it does not decide what the prescription should do with a body excited past a closed orbit, which is a question about the model rather than about its bookkeeping.
The excitation applied to a pair before a collision is drawn without an upper bound, and neither is the fallback taken when no draw satisfies the overlap condition, so a body can enter the collision branch already past unity eccentricity while the ejection branch treats that as grounds for removal. The two branches disagree about what an unbound orbit means. Capping the values in the record was the wrong answer. A body that acquires an unbound eccentricity is removed on the same step by the perihelion cut and never survives to be reported, so the histories run_system returns carry no such value: across 432 systems and 2424 delivered records spanning the model's usual mass, spacing and separation ranges, not one record reached the cap. What the cap did reach was the case where only the pre-collision value needed it, and there it replaced a true eccentricity with a fixed one, so the change a consumer applies came out wrong by an unbounded amount with only a warning to say so. A quietly wrong number is worse than the rejection it was avoiding. The behaviour is recorded in the limitations instead, where the caveat belongs: it is visible only to a caller reading the raw merger list rather than the returned histories, and what to do about the disagreement is a question about the prescription rather than about the bookkeeping.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Adds
e_beforeto each impact record, so both orbital elements are reported on either side of a collision, and turns on formatter enforcement in CI, which was written into the contributor guide but checked nowhere.This changes the PROTEUS record schema and is a breaking interface change. PROTEUS reads the fields by name and refuses a record missing any of them, so a PROTEUS accretion run needs a release carrying
e_before.Why
An impact record carried the target's semi-major axis on both sides of the collision but its eccentricity only after. PROTEUS follows a planet that generally sits on a different orbit from the recorded body, so it applies the semi-major axis as the fractional change the collision made rather than transplanting an absolute value that belongs to this system. It had no way to do the same for the eccentricity, so it overwrote whatever the user configured. Tidal heating scales as the square of eccentricity, so that was a large change applied silently: a planet configured at 0.1 was reset to 0.004 at the first impact, and with tides off the orbit is re-pinned from the configuration every step, so it never recovered.
Reporting the eccentricity on both sides lets a consumer apply the difference, which is the eccentricity's analogue of the ratio already used for the semi-major axis. A difference rather than a ratio because eccentricity is dimensionless and routinely zero.
Separately, linting was enforced and formatting was not, so style had drifted: sixteen of the twenty-four source and test files did not match the formatter, and the pre-commit hook for it was present but commented out.
Changes
orbit_cross_K25.py: capture the target's eccentricity before the merger overwrites it, and report it ase_before.driver.py: carrye_beforeinto the per-impact record.ruff format --checkalongsideruff check; the pre-commit hook is enabled. The generated_version.pyis excluded, since the build writes it in its own style to an untracked path, so no contributor could bring it into line.Testing
ruff checkandruff format --checkboth clean oversrc/,tests/,tools/.e_before. Worth noting what it pins: the recorded value is the eccentricity at the moment of the collision, not the value the system started from. Viscous stirring excites the orbits first, so a pair seeded at 0.05 collides at 0.207. That is the geometry the collision actually happened at, which is what a consumer reconstructing the impact needs. The test also asserts it differs frome_after, since reading the same array one line later would report the merged value.