refactor!: profile target renaming #526
Conversation
apaletta3
commented
Feb 17, 2025
apaletta3
commented
- Renaming elements of the Profile class including classes within its namespace and method to more accurately reflect certain terminology in the attitude world
- Fixed a unit test in python for profile
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Overall I agree we can improve some of the naming here. Let's be careful though to not make another round of breaking changes unless absolutely necessary, especially if it's just naming. We can make a Github issue highlighting the naming we would like in the future and then do it in one big refactor
| OrbitalMomentum, /// The orbital momentum vector of the satellite in the ECI frame | ||
| OrientationProfile, /// Points towards a profile of orientations in the ECI frame | ||
| Custom, /// Custom target | ||
| // Simple pre-defined targets |
There was a problem hiding this comment.
Not sure if this comment adds much value, I think it's better to extend the explanations in the enums so that they appear as docstrings as well.
|
|
||
| // More complex targets | ||
| Trajectory, /// Points towards the provided trajectory, eg. Ground Station in ECEF or another satellite | ||
|
|
||
| // Custom pointing | ||
| CustomProfileECI, /// Pointing pre-defined via a profile of vectors in the ECI frame | ||
| CustomProfileGeneratorECI, /// Pointing generated at each timestep by a function of that returns a vector in | ||
| /// the ECI frame |
There was a problem hiding this comment.
| // More complex targets | |
| Trajectory, /// Points towards the provided trajectory, eg. Ground Station in ECEF or another satellite | |
| // Custom pointing | |
| CustomProfileECI, /// Pointing pre-defined via a profile of vectors in the ECI frame | |
| CustomProfileGeneratorECI, /// Pointing generated at each timestep by a function of that returns a vector in | |
| /// the ECI frame | |
| Trajectory, /// Points towards the provided trajectory, eg. Ground Station in ECEF or another satellite | |
| Custom /// Pointing generated at each timestep by a function that returns a Quaternion in the ECI frame |
Not sure if we want 2 different custom profiles here? They're both just technically generators?
| /// @brief Represents a target for alignment or pointing purposes. | ||
| class Target | ||
| /// @brief Represents pointing of one axis of your spacecraft towards a target. | ||
| class Pointing |
There was a problem hiding this comment.
I agree target might not have been the best word, but let's remember that it's accessed via the Profile class so it's Profile.Target.
I don't think we should call it pointing as if you read the brief you're basically saying pointing of one axis of your spacecraft towards a **target**
