Skip to content

The grain's outer radius was smaller than its bore - #2

Open
thc1006 wants to merge 1 commit into
ARRC-Rocket:mainfrom
thc1006:fix/grain-geometry-outer-radius
Open

The grain's outer radius was smaller than its bore#2
thc1006 wants to merge 1 commit into
ARRC-Rocket:mainfrom
thc1006:fix/grain-geometry-outer-radius

Conversation

@thc1006

@thc1006 thc1006 commented Aug 8, 2026

Copy link
Copy Markdown
Member

The grain's outer radius is smaller than its bore, so the motor is carrying negative fuel.

grain_outer_radius          0.00843     official scenario: 0.0843
grain_initial_inner_radius  0.0295      official scenario: 0.0295

A hollow grain's mass goes as outer^2 - inner^2. With the outer radius under the inner one that is negative, and RocketPy 1.13 builds the motor without a warning:

solid grain mass    -0.623 kg   ->   4.861 kg
total propellant    15.407 kg   ->  20.891 kg
rocket mass at t=0  85.407 kg   ->  90.891 kg
apogee               199.8 m    ->   130.5 m
max speed            152.6 m/s  ->   122.5 m/s

The apogee was 53% high because the rocket weighed 5.5 kg less than it should. Every other parameter in this file already matches scenario_1_parameters.yaml, which is what makes this one stand out.

The check. Nothing downstream catches it, so the radii are now named constants with a guard in front:

if not 0 < inner_radius < outer_radius:
    raise ValueError(...)

It rejects an inner radius that is zero, negative, equal to the outer one, or larger. raise rather than assert, so it survives python -O.

One more, and it changes nothing today. center_of_dry_mass_position is 0.015 where the scenario has 0.15. It is inert while dry_mass is 0, because it is a mass-weighted position, and I checked: total mass and centre of mass come out bit-identical either way. Aligned anyway, since it is the same decimal slip and it stops being inert the day dry_mass is not zero. Happy to drop that hunk if the value was deliberate.

Verified with the repo's own gates: ruff check and ruff format --check clean, and python simulation.py runs end to end and writes report.html.

grain_outer_radius was 0.00843 where the official scenario has 0.0843. A hollow
grain's mass goes as (outer^2 - inner^2), so an outer radius under the inner one
is a negative mass, and RocketPy 1.13 builds it without a word:

  solid grain mass   -0.623 kg  ->  4.861 kg
  total propellant   15.407 kg  ->  20.891 kg
  apogee              199.8 m   ->  130.5 m

The apogee was 53% high because the rocket was carrying negative fuel.

Since nothing downstream will catch this, the radii are now named constants with
a check in front of them. It rejects an inner radius that is zero, negative,
equal to the outer one, or larger.

Also center_of_dry_mass_position, 0.015 where the scenario has 0.15. That one is
inert today because dry_mass is 0 and it is a mass-weighted position, so it
changes no number in this diff. Aligned anyway: it is the same decimal slip, and
it stops being inert the day dry_mass does not stay zero.

Every other parameter in this file already matches the official scenario.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006

thc1006 commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

The red is the last step only, and it is not the physics. Step by step:

Lint                        success
Format check                success
Run script                  success     <- the simulation ran and produced the report
Upload report.html          success     <- the report is attached to this run
Comment on PR               failure

The failure:

HttpError: Resource not accessible by integration
create-an-issue-comment, status: 403

That step uses GITHUB_TOKEN to post a comment. This repository is public and this pull request comes from a fork, so the token is read-only for the whole job. The permissions: pull-requests: write block cannot raise that: it can only narrow what the event already grants, and a pull_request run from a fork is not granted write.

Every earlier run on this workflow came from enh/setup-basics, a branch on the repository itself, which does get a write token. So the step has never been exercised from a fork, and this pull request is the first one to reach it.

The report itself is fine. It is on the run as the report artifact; only the comment that would have linked to it is missing.

Two ways to close it, both small:

# skip the comment for fork pull requests
if: github.event_name == 'pull_request' && !github.event.pull_request.head.repo.fork

# or let it fail without failing the job
continue-on-error: true

I would take the first, since a step that cannot succeed is better skipped than tolerated. Happy to send it as its own pull request, though note it would show the same red until it lands, for the same reason.

@zuorenchen your call on whether that goes in now or after tomorrow.

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.

1 participant