Skip to content

Output Docs Review #1152

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

Merged
merged 19 commits into from
Jun 3, 2025
Merged

Output Docs Review #1152

merged 19 commits into from
Jun 3, 2025

Conversation

Gerenjie
Copy link
Collaborator

@Gerenjie Gerenjie commented Jun 1, 2025

Finally getting around to my output docs job…

  • The columns object_linked and date_linked_MJD are missing from the [] but incorrectly present in the The Basic Output Column Names table lists, so I’ve moved them.
  • fieldID was missing from the ‘all’ and ephemeris output columns, so I’ve added it.
  • The Example Detections File in Basic Format appear quite different from what I get in the recent run, so I’ve replaced it with the top ten lines of the demo output.
  • The stats file does not appear to include object_linked so I’ve updated the docs to match. It does include date_linked_MJD, though.
  • A description of -ew output columns is missing both from this and from the “Ephemeris Generation” page, so I’ve added it here.

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 Sorcha 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?

Fixes #1114

@Gerenjie Gerenjie requested a review from mschwamb June 1, 2025 03:06
@@ -305,8 +309,6 @@ Statistics (Tally) File Column Names, Formats, and Descriptions
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| min_phase | Float | Maximum calculated phase angle for this object in this filter (degrees) |
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| object_linked | Boolean | True/False whether the object was linked by SSP (only included if linking is on) |
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| date_linked_MJD | Float | Date the object was linked (if it was linked) in MJD (only included if linking is on) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this stays in it shows up if the drop_unlinked = False

docs/outputs.rst Outdated
@@ -189,19 +189,19 @@ Detections File: Full Output Column Names, Formats, and Descriptions
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| phase_deg | Float | The sun-object-observer angle (degrees) |
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| epochMJD_TDB | Float | Epoch of orbit (MJD) in Barycentric Dynamical Time |
| q | Float | Object perihelion (au) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

this depends on what you're doing - I don't have a q because I inputted Keplerian orbit - so I think we need to say orbital elements

docs/outputs.rst Outdated
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| t_p_MJD_TDB | Float | Time of periapsis (MJD) in Barycentric Dynamical Time |
| e | Float | Orbital eccentricity |
Copy link
Collaborator

Choose a reason for hiding this comment

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

this depends on what you're doing - I don't have a q because I inputted Keplerian orbit - so I think we need to say orbital elements

docs/outputs.rst Outdated
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| argPeri | Float | Argument of periaspsis of the object (degrees) |
| inc | Float | Orbital inclination (degrees) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

this depends on what you're doing - I don't have a q because I inputted Keplerian orbit - so I think we need to say orbital elements

docs/outputs.rst Outdated
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| node | Float | Longitude of the ascending node of the object (degrees) |
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| inc | Float | Orbital inclination (degrees) |
| argPeri | Float | Argument of periaspsis of the object (degrees) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

this depends on what you're doing - I don't have a q because I inputted Keplerian orbit - so I think we need to say orbital elements

docs/outputs.rst Outdated
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| e | Float | Orbital eccentricity |
| t_p_MJD_TDB | Float | Time of periapsis (MJD) in Barycentric Dynamical Time |
Copy link
Collaborator

Choose a reason for hiding this comment

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

this depends on what you're doing - I don't have a q because I inputted Keplerian orbit - so I think we need to say orbital elements

+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| q | Float | Object perihelion (au) |
| epochMJD_TDB | Float | Epoch of orbit (MJD) in Barycentric Dynamical Time |
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is always there, but part of orbital elements

+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| q | Float | Object perihelion (au) |
| epochMJD_TDB | Float | Epoch of orbit (MJD) in Barycentric Dynamical Time |
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| FORMAT | Float | Orbit format string (COM for heliocentric, BCOM for barycentric, KEP for Keplerian, CART for Cartesian) |
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing as above we might want to say orbits

| object_linked | Boolean | True/False whether the object passed the linking filter. See note below |
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+
| date_linked_MJD | Boolean | MJD (TAI) Date the object was linked (if it was linked) See note below |
+------------------------------------+--------------+----------------------------------------------------------------------------------------------------------+

.. note::
All positions, positions, and velocities are in respect to J2000.
Copy link
Collaborator

@mschwamb mschwamb Jun 1, 2025

Choose a reason for hiding this comment

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

Let's have a note about object_linked and date_linked_MJD here - object_linked only shows up if the user has drop_unlinked=True in full output

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After some tests, I see them both showing up for both values of object_linked and have changed the docs to match.


.. note::
All columns in the comple physicalx parameters file will also be included in the full output.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have this under the inputs section - we should directly link there, but I think better to not duplicate here otherwise we have things to maintain - check the inputs is correct - I am pretty sure it is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure how to do links in documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can add it in once this PR goes in

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe I've responded to / fixed according to all your comments -- is there anything else blocking this review?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just me having waking hours to do it - working on it now

Copy link
Collaborator

@mschwamb mschwamb left a comment

Choose a reason for hiding this comment

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

Thanks fort doing this and spotting typos and missing columns - a few suggested edits/clarifications and a request about the ephemeris output

@Gerenjie
Copy link
Collaborator Author

Gerenjie commented Jun 2, 2025

I've made changes in response to your comments.

@mschwamb mschwamb self-requested a review June 3, 2025 09:51
@mschwamb mschwamb merged commit 89208b1 into main Jun 3, 2025
7 checks passed
@mschwamb mschwamb deleted the jake_outputs_review branch June 3, 2025 09:56
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.

Documentation Review
2 participants