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

Sentinel2 examples #38

Merged
merged 44 commits into from
Nov 30, 2022
Merged

Sentinel2 examples #38

merged 44 commits into from
Nov 30, 2022

Conversation

GabrieleMeoni
Copy link
Collaborator

Description

Summary of changes

  • Added Sentinel2 example notebook and utils.
  • Edited activity manager to handle the execution in a notebook.
  • Edited paseos initialization to enable initialization for t != 0.

Resolved Issues

  • fixes #Issue

How Has This Been Tested?

The notebook functionalities work as expected.

Related Pull Requests

@gomezzz
Copy link
Collaborator

gomezzz commented Nov 14, 2022

@GabrieleMeoni maybe you can merge #39 that should fix tests and update plotting to latest status

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

environment.yml Outdated Show resolved Hide resolved
paseos/__init__.py Outdated Show resolved Hide resolved
examples/Sentinel_2_example_notebook/utils.py Show resolved Hide resolved
examples/Sentinel_2_example_notebook/utils.py Outdated Show resolved Hide resolved
@gomezzz gomezzz self-requested a review November 14, 2022 15:30
@gomezzz gomezzz added documentation Improvements or additions to documentation visualization Code used to visualize the simulation example An example to showcase something user-facing Anything that users can interact with labels Nov 14, 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.

- Add vis of running activities
@gomezzz gomezzz mentioned this pull request Nov 28, 2022
1 task
@@ -0,0 +1,616 @@
{
Copy link
Collaborator

@gomezzz gomezzz Nov 30, 2022

Choose a reason for hiding this comment

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

Can you add. "S2B will be the local actor, i.e. the one paseos models. In this example, we only model this one satellite."


Reply via ReviewNB

@@ -0,0 +1,616 @@
{
Copy link
Collaborator

@gomezzz gomezzz Nov 30, 2022

Choose a reason for hiding this comment

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

Can you add: "Internally, paseos uses pykep.planet objects to model gravitational acceleration and Keplerian orbits."


Reply via ReviewNB

@@ -0,0 +1,616 @@
{
Copy link
Collaborator

@gomezzz gomezzz Nov 30, 2022

Choose a reason for hiding this comment

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

Line #1.    #Define today as pykep epoch (27-10-22)

can you add

# see also https://esa.github.io/pykep/documentation/core.html#pykep.epoch


Reply via ReviewNB

@@ -0,0 +1,616 @@
{
Copy link
Collaborator

@gomezzz gomezzz Nov 30, 2022

Choose a reason for hiding this comment

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

SpacecraftActor not space actor

This is not correct though. It is already a SpacecraftActor, it just doesn't have an orbit. So would rephrase into something like: "Now we define the actor's orbit."


Reply via ReviewNB

@@ -0,0 +1,616 @@
{
Copy link
Collaborator

@gomezzz gomezzz Nov 30, 2022

Choose a reason for hiding this comment

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

Line #1.    #Adding orbits around Earth based on previously calculated ephemerides

->

Line #1.    #Adding orbit around Earth based on previously calculated ephemerides



Reply via ReviewNB

@@ -0,0 +1,616 @@
{
Copy link
Collaborator

@gomezzz gomezzz Nov 30, 2022

Choose a reason for hiding this comment

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

status in which the satellite is an await status before

grammar

status in which the satellite is in before collecting new images?


Reply via ReviewNB

@@ -0,0 +1,616 @@
{
Copy link
Collaborator

@gomezzz gomezzz Nov 30, 2022

Choose a reason for hiding this comment

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

activity wait_before_acquire_async(time_s)

This is not the activity but the activity_func

utilities for long time

"for a longer time"


Reply via ReviewNB

@@ -0,0 +1,616 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also use the function on_termination

We also use a function on_termination (...)

Termination functions are optional but useful if you want to perform something after an activity.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why a? Is the function that we define here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The thing I dislike is that it is confusing whether you mean the parameter on_termination_function of register_activity or the function on_termination because you gave it the same name . It would be better, e.g., if you call it finish_volcano_detection and then explain that you can execute something when an activity finishes by setting a on_termination_function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(same actually for the constraint function to some degree)

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 removed it as you suggested before. My comment was on the grammar (a vs the)

@@ -0,0 +1,616 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I would remove this since we don't do anything in the function. Can be in another example. We don't need to explain everything at once.


Reply via ReviewNB

@@ -0,0 +1,616 @@
{
Copy link
Collaborator

@gomezzz gomezzz Nov 30, 2022

Choose a reason for hiding this comment

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

Line #3.        "wait_before_acquire", 

Can we call it something more comprehensible?

"Standing by" ?


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Standing by looks way more cryptical to me. I called it idle_state

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.

see comments on https://app.reviewnb.com/aidotse/PASEOS/pull/38/ , mostly some text changes

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.

👍

@GabrieleMeoni GabrieleMeoni merged commit 08f67d7 into main Nov 30, 2022
@GabrieleMeoni GabrieleMeoni deleted the sentinel2_examples branch November 30, 2022 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation example An example to showcase something user-facing Anything that users can interact with visualization Code used to visualize the simulation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants