Skip to content

Conversation

@dhutererprats
Copy link

@dhutererprats dhutererprats commented Dec 11, 2025

  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

The variable shadowFactor is used to determine if a satellite is in the shadow of a planet, and if yes, by how much. A value of 1 meant the satellite was outside the shadow, and a value of 0 meant it was fully in the shadow. However, the name implies the opposite behavior. Thus, we are changing this variable name to reflect better describe what it does.
This PR adopts a clearer name for the eclipse payload field: illuminationFactor. For backward compatibility, shadowFactor continues to work and is formally deprecated until Dec 31, 2026.

C / C++

  • EclipseMsgPayload:
    • illuminationFactor is the new primary variable.
    • shadowFactor remains as a deprecated alias.
  • SpacecraftLocation:
    • min_illumination_factor is the new primary variable.
    • min_shadow_factor remains as a deprecated public variable, initialized to a sentinel value (-2.0).
    • Logic Update: In Reset(), the code checks if min_shadow_factor != -2.0. If true, it detects that the user has explicitly set the legacy variable. It then maps this value to min_illumination_factor and logs a BSK_WARNING, ensuring valid configurations are preserved without overwriting new defaults.

Python / SWIG

  • EclipseMsgPayload.illuminationFactor is exposed as a property that delegates to the same getter/setter as shadowFactor (no behavior or performance changes).

  • EclipseMsgPayload.shadowFactor remains available but:

    • Emits a deprecation warning (via Basilisk.utilities.deprecated.deprecationWarn) up to 2026-12-31.
    • Is intended to be removed after that date.
  • EclipseMsgRecorder (from eclipseMsg.recorder()) now also supports:

    recorder.illuminationFactor → alias for recorder.shadowFactor

    so log access uses the new name without breaking existing code.

Modules / Files Touched

  • architecture/msgPayloadDefC/EclipseMsgPayload.h
    • Adds illuminationFactor alias and associated comments.
  • architecture/messaging/msgAutoSource/msgInterfacePy.i.in
    • Adds Python illuminationFactor property on EclipseMsgPayload.
    • Wraps shadowFactor with deprecation behavior.
    • Extends EclipseMsgRecorder to handle illuminationFactor in logs.
  • Eclipse-related environment/sensor modules
    • Call sites updated to use illuminationFactor while remaining compatible with shadowFactor.

Verification

  • Full Basilisk build passes.
  • All eclipse unit tests still pass
  • New regression test:
    • test_shadow_vs_illumination_alias_and_deprecation_behavior validates:
      • illuminationFactor and shadowFactor stay numerically in sync.
      • Before the cutoff date, using shadowFactor emits a deprecation warning (checked with pytest.warns).
      • After the cutoff date, the test expects shadowFactor to raise an error while illuminationFactor continues to work.

Documentation

  • Updated RST documentation for eclipse and related modules (e.g., solarFlux, coarseSunSensor, albedo, etc.) to:
    • Introduce illuminationFactor as the preferred name.
    • Mark shadowFactor as deprecated and scheduled for removal after Dec 31, 2026.
    • Add a short migration note showing the old vs. new name and the deprecation timeline.

Future work

  • On/after 2026-12-31:
    • Remove shadowFactor from EclipseMsgPayload.
    • Remove min_shadow_factor from SpacecraftLocation.
    • Remove Python aliases and warning logic.

@dhutererprats dhutererprats self-assigned this Dec 11, 2025
@dhutererprats dhutererprats requested a review from a team as a code owner December 11, 2025 07:14
@dhutererprats dhutererprats moved this to ✅ Done in Basilisk Dec 11, 2025
@dhutererprats dhutererprats added the enhancement New feature or request label Dec 11, 2025
@schaubh schaubh changed the title Feature/illumination factor renaming Rename shadow factor to illumination factor Dec 11, 2025
@schaubh schaubh moved this from ✅ Done to 👀 In review in Basilisk Dec 11, 2025
\caption{Definition and Explanation of Variables Used.}
\centering \fontsize{10}{10}\selectfont
\begin{tabular}{ | m{5cm}| m{2cm} | m{1.5cm} | m{6cm} |} % Column formatting,
\begin{tabular}{ | m{5cm}| m{2cm} | m{1.5cm} | m{6cm} |} % Column formatting,
Copy link
Contributor

Choose a reason for hiding this comment

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

if you update the TeX you also need to compile the TeX document and commit the updated PDF.

Copy link
Author

Choose a reason for hiding this comment

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

this was just deleting a trailing whitespace in the comment of the line and should not affect the pdf.

Copy link
Contributor

Choose a reason for hiding this comment

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

You updated the TeX file, but didn't upload a new PDF file? Please upload the current PDF file.
At the top of the TeX file you should also add an entry to the revision history to say what change you made.

@dhutererprats dhutererprats force-pushed the feature/illumination_factor_renaming branch from b2de988 to 0946e61 Compare December 22, 2025 10:51
@dhutererprats dhutererprats force-pushed the feature/illumination_factor_renaming branch 2 times, most recently from 14ea036 to e8d7c3d Compare January 3, 2026 21:16
@dhutererprats dhutererprats force-pushed the feature/illumination_factor_renaming branch from 2ea00ff to eee3a25 Compare January 7, 2026 11:13
@dhutererprats dhutererprats requested a review from schaubh January 7, 2026 13:14
Copy link
Contributor

@schaubh schaubh left a comment

Choose a reason for hiding this comment

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

Almost there. You didn't update the TeX PDF file of the SolarRadiation Pressure module.

\caption{Definition and Explanation of Variables Used.}
\centering \fontsize{10}{10}\selectfont
\begin{tabular}{ | m{5cm}| m{2cm} | m{1.5cm} | m{6cm} |} % Column formatting,
\begin{tabular}{ | m{5cm}| m{2cm} | m{1.5cm} | m{6cm} |} % Column formatting,
Copy link
Contributor

Choose a reason for hiding this comment

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

You updated the TeX file, but didn't upload a new PDF file? Please upload the current PDF file.
At the top of the TeX file you should also add an entry to the revision history to say what change you made.

@dhutererprats dhutererprats force-pushed the feature/illumination_factor_renaming branch from eee3a25 to 6a61cfa Compare January 9, 2026 21:20
@dhutererprats dhutererprats force-pushed the feature/illumination_factor_renaming branch from 6a61cfa to 57d74fc Compare January 9, 2026 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

4 participants