Skip to content
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

First steps module implementation #3

Merged
merged 15 commits into from
Oct 10, 2022
Merged

Conversation

gomezzz
Copy link
Collaborator

@gomezzz gomezzz commented Oct 6, 2022

Description

Summary of changes

  • Added orbit computation
  • Several smaller fixes and renaming of the roject
  • Added actor system for spacecraft / groundstations
  • Tests for various new parts
  • Line of sight computation
  • Ignored flake8 things that were incompatible with black

Resolved Issues

N/A

How Has This Been Tested?

  • Included new tests

Related Pull Requests

N/A

@gomezzz gomezzz added enhancement New feature or request tests Anything related to the tests labels Oct 6, 2022
@gomezzz gomezzz marked this pull request as ready for review October 7, 2022 14:42
Copy link
Collaborator

@johanos1 johanos1 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Collaborator

@GabrieleMeoni GabrieleMeoni left a comment

Choose a reason for hiding this comment

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

I tried to executed the line of sight test and works correctly for me. The code is pretty straightforward. Could you, please, just clarify who is the origin (0,0,0)? I guess the code should work independently, but maybe add a comment on this somewhere.

p1.plotter(c="r", s=100),
p2.plotter(c="r", s=100),
)
except ValueError:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why ValueError?

Copy link
Collaborator Author

@gomezzz gomezzz Oct 10, 2022

Choose a reason for hiding this comment

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

See comment in L107

# Currently skspatial throws a ValueError if there is no intersection so we have to use this rather ugly way.


Args:
name (str): Name of this actor
position (list of floats): [x,y,z]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks odd that Groundstation has a "velocity". I guess you are doing that because our (0,0,0) is not necessarily the ground station. Maybe, add a comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Everything is in central body frame, the ground station is "orbiting" it with the Earth's (or other body's) rotation as velocity and position relative to the center. Added a comment to clarify


# check positions one second later
r, v = actor.get_position_velocity(pk.epoch(1 * pk.SEC2DAY))
assert np.isclose(r[0], 999800.6897266058)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you, please, add some comments on the meaning of these magic numbers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

r,v are commonly position velocity vector in astrodynamics, added a comment to clarify

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(the numbers are just values I determined trying out, there is #1 to make more robust tests)

@gomezzz gomezzz merged commit 02ed942 into main Oct 10, 2022
@gomezzz gomezzz deleted the first_steps_module_implementation branch October 10, 2022 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request tests Anything related to the tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants