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

Visual implementation #27

Merged
merged 44 commits into from
Nov 22, 2022
Merged

Visual implementation #27

merged 44 commits into from
Nov 22, 2022

Conversation

johanos1
Copy link
Collaborator

@johanos1 johanos1 commented Oct 23, 2022

Description

Summary of changes

  • A class to visualize the actors and central object in paseos

Resolved Issues

  • fixes #Issue

How Has This Been Tested?

  • Examples in visualization_example have been examined by human
  • Test added to paseos/tests without creating plot window. Makes sure the function calls are OK.

Related Pull Requests

  • #PR

@johanos1 johanos1 added enhancement New feature or request feature implementing a new feature labels Oct 24, 2022
Copy link
Collaborator

@gomezzz gomezzz left a comment

Choose a reason for hiding this comment

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

A few comments, currently I can't run it yet on my machine because of imports etc.

paseos/actors/spacecraft_actor.py Outdated Show resolved Hide resolved
paseos/actors/spacecraft_actor.py Outdated Show resolved Hide resolved
paseos/paseos.py Outdated Show resolved Hide resolved
paseos/paseos.py Outdated Show resolved Hide resolved
paseos/visualization/animation.py Show resolved Hide resolved
paseos/visualization/space_animation.py Outdated Show resolved Hide resolved
paseos/visualization/space_animation.py Outdated Show resolved Hide resolved
paseos/visualization/space_animation.py Show resolved Hide resolved
paseos/visualization/space_animation.py Outdated Show resolved Hide resolved
paseos/visualization/space_animation.py Outdated Show resolved Hide resolved
@gomezzz
Copy link
Collaborator

gomezzz commented Oct 24, 2022

I'd also suggest to merge main into this first given the changes from #26

@gomezzz
Copy link
Collaborator

gomezzz commented Oct 25, 2022

Ah, one more thought: Maybe we can add a minimal example of this to the README.md with a screenshot of what it looks like? This seems like the kind of feature user would really want to know how to use :)

@johanos1 johanos1 requested a review from gomezzz October 25, 2022 10:58
@gomezzz gomezzz changed the base branch from main to extend_activities October 25, 2022 12:04
@gomezzz gomezzz changed the base branch from extend_activities to main October 25, 2022 12:04
Copy link
Collaborator

@gomezzz gomezzz left a comment

Choose a reason for hiding this comment

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

a few more comments, in general I still think it would be nice to add a minimal example (like below) with a screenshot to the readme.md for users

I still can't run it outside the module with e.g.

import paseos
import pykep as pk
from paseos import SpacecraftActor,ActorBuilder
earth = pk.planet.jpl_lp('earth')
# Define local actor
sat1 = ActorBuilder.get_actor_scaffold(
    "sat1", SpacecraftActor, [10000000, 0, 0], pk.epoch(0)
)
ActorBuilder.set_orbit(sat1, [10000000, 0, 0], [0, 8000.0, 0], pk.epoch(0), earth)
ActorBuilder.set_power_devices(sat1, 500, 10000, 1)
# init simulation
sim = paseos.init_sim(sat1)

from paseos import SpaceAnimation
anim = SpaceAnimation(sim)
anim.animate(sim, "paseos_test", 200, 400)

I get [Errno 2] No such file or directory: '/home/pablo/ESA/PASEOS/my_notebooks/FontAwesome.otf' due the path not being relative (see here how to do it:

os.path.dirname(__file__) + "/../../resources/", "default_cfg.toml"
)

paseos/__init__.py Outdated Show resolved Hide resolved
paseos/actors/spacecraft_actor.py Outdated Show resolved Hide resolved
paseos/actors/spacecraft_actor.py Outdated Show resolved Hide resolved
paseos/paseos.py Outdated Show resolved Hide resolved
paseos/paseos.py Outdated Show resolved Hide resolved
paseos/visualization/space_animation.py Outdated Show resolved Hide resolved
paseos/visualization/space_animation.py Outdated Show resolved Hide resolved
paseos/visualization/space_animation.py Outdated Show resolved Hide resolved
paseos/visualization_test/test.py Outdated Show resolved Hide resolved
paseos/visualization_test/test.py Outdated Show resolved Hide resolved
@gomezzz gomezzz mentioned this pull request Oct 26, 2022
9 tasks
@johanos1 johanos1 requested a review from gomezzz October 30, 2022 11:41
Copy link
Collaborator

@gomezzz gomezzz left a comment

Choose a reason for hiding this comment

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

Good news:

  • For me I get the exact same experience on Win & Linux
  • In both cases, in a jupyter notebook works

Bad news:

On my windows machine, if I run the example/visualization/example_animation.py I get this

image

On Linux it doesn't even open the window and just shows me briefly a blank screen window at the end.

But I have one more fundamental question, because I am starting to wonder how we interface this with the activities and user actions. Because then, you don't want to call something that blocks you from interacting with paseos?

It should be more like:

# Some setup code
# plot the current animation frame
paseos.plot()

# user can do some actions, activities whatever they want

# plot the next frame
paseos.plot( )

What do you think? @johanos1

@@ -21,7 +21,7 @@ def is_in_eclipse(
Returns:
bool: True if actor is in eclipse
"""
logger.debug(f"Checking whether {actor} is in eclipse at {t}.")
logger.debug(f"Checking whether {actor} is in eclipse at {t.mjd2000/pk.SEC2DAY}.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert 🙏

paseos/visualization/space_animation.py Outdated Show resolved Hide resolved
examples/visualization/example_jupyter.ipynb Outdated Show resolved Hide resolved
@gomezzz
Copy link
Collaborator

gomezzz commented Nov 1, 2022

let's highlight local actor in special color

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@gomezzz gomezzz self-requested a review November 16, 2022 10:05
Copy link
Collaborator

@gomezzz gomezzz left a comment

Choose a reason for hiding this comment

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

some minor changes

README.md Show resolved Hide resolved
paseos/paseos.py Outdated
@@ -95,7 +95,6 @@ def add_known_actor(self, actor: BaseActor):
@property
def local_actor(self) -> BaseActor:
"""Returns the local actor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert these white space changes. There should be an empty line after description to follow https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

@@ -21,7 +21,7 @@ def is_in_eclipse(
Returns:
bool: True if actor is in eclipse
"""
logger.debug(f"Checking whether {actor} is in eclipse at {t}.")
logger.debug(f"Checking whether {actor} is in eclipse at {t.mjd2000/pk.SEC2DAY}.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still open

paseos/visualization/plot.py Show resolved Hide resolved
@gomezzz
Copy link
Collaborator

gomezzz commented Nov 18, 2022

We also need to explain somewhere what you actually see in the visualization. Especially what the blue ball is, what the green / red matrix is and why some actors have different colors. Probably in the readme? 🤔

Copy link
Collaborator

@gomezzz gomezzz left a comment

Choose a reason for hiding this comment

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

👍 Only very minor string changes in readme I would still apply 🙏 We did it, yay! 🛰️ 🚀

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
johanos1 and others added 4 commits November 22, 2022 10:17
Co-authored-by: Pablo Gómez <contact@pablo-gomez.net>
Co-authored-by: Pablo Gómez <contact@pablo-gomez.net>
Co-authored-by: Pablo Gómez <contact@pablo-gomez.net>
Co-authored-by: Pablo Gómez <contact@pablo-gomez.net>
@johanos1 johanos1 merged commit 4ea4fb2 into main Nov 22, 2022
@gomezzz gomezzz deleted the visual_implementation branch December 1, 2022 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature implementing a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants