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

add example #57

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

add example #57

wants to merge 6 commits into from

Conversation

HansVRP
Copy link
Contributor

@HansVRP HansVRP commented Nov 19, 2024

No description provided.

Copy link
Contributor

@VictorVerhaert VictorVerhaert left a comment

Choose a reason for hiding this comment

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

I would combine section 1 and 2 as this is a basic openeo setup method (I would assume that by the time someone looks at visualizing job manager results they know these steps)

Section 3 is a bit incosistent. First a transformer is used and then the other transormation is done using geodataframe. To prevent confusion I would stick to one projection method.
In the description I would make it clear we want to have a grid of 5km first as simply starting the projections explanation is confusing as to why you are doing this (at least to me). I would also but the variable definitions first in the code block as good practise (min max lonlat and the grid_size_m).

section 5: the new python client has functionality to initiate a job df (factory function called create_job_db I think) which would be nice to showcase as well

Section 6 title is confusing as the jobs are not yet run

I would group all de dependencies at the start of the notebook as these clutter the notebook.

Some comments in the codeblocks still refence subtitles that are used elsewhere.

As this notebook uses some non-default-openeo packages it might be good to either make a requerements file or name them at the start with bash command ("pip install plotly")

there is an empty code clock at the end of the file

@HansVRP
Copy link
Contributor Author

HansVRP commented Nov 19, 2024

good feedback, I was working on the cleanup, I am also trying to figure out how I can render the plotly plots in a static github environment

@HansVRP
Copy link
Contributor Author

HansVRP commented Dec 11, 2024

@VictorVerhaert reminder

Copy link
Contributor

@VictorVerhaert VictorVerhaert left a comment

Choose a reason for hiding this comment

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

Next to the specific comments the entry in the readme should be elaborated so users can find the example and perhaps adding a readme in the managingmultiplelargescalejobs folder to indicate the difference between the two notebooks.
processes I would definitly mention is using apex processes (might need a separate simple example as well), threaded job manager, and ofcourse visualisation

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this file should be included as it is a result of running the code

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this file should be included as it is a result of running the code

Copy link
Contributor

Choose a reason for hiding this comment

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

The first code block still has some result logs from installing the packages, remove those to make the notebook cleaner

first title is still authenticate but that only happens in block 4

in section 2 the tiling grid is saved as a geojson and it is read again as a gdf, could we skip the writing of the file and simply save the result of create_tiling_grid directly?

in section 3: prepare_jobs_df simply adds the temporal range as a column to the grid_df. This can be done by grid_df["temporal_extent"] = ["2024-05-01", "2024-08-01"] which is less confusing than iterating over all the rows.

description of section 3 mentions the best available composits (which should be best available pixel I think) but this is only done in section 4.

the default polling frequency of the MBJM is 60 seconds (can be set during init of the JM) but the visuals refresh every 30 seconds. Exposing this argument and syncing them might be better.

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.

2 participants