Skip to content

Generate Presto embeddings in Google Earth Engine#41

Merged
ivanzvonkov merged 6 commits intomainfrom
initial-deploy-code
May 26, 2025
Merged

Generate Presto embeddings in Google Earth Engine#41
ivanzvonkov merged 6 commits intomainfrom
initial-deploy-code

Conversation

@ivanzvonkov
Copy link
Copy Markdown
Contributor

@ivanzvonkov ivanzvonkov commented May 16, 2025

The PR includes a notebook for deploying Presto to Vertex AI and a Google Earth Engine script for generating Presto embeddings using the Vertex AI model.

Direct link to notebook: https://github.com/nasaharvest/presto/blob/initial-deploy-code/deploy/1_Presto_to_VertexAI.ipynb

  • Update Colab Button (in README and notebook) to main branch prior to merging.

@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ivanzvonkov ivanzvonkov requested a review from gabrieltseng May 16, 2025 19:35
@@ -0,0 +1,631 @@
{
Copy link
Copy Markdown
Collaborator

@gabrieltseng gabrieltseng May 26, 2025

Choose a reason for hiding this comment

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

It may be worth doing something to highlight 5c more. It would be a bummer if someone distracted started incurring costs. Maybe a ⚠️ emoji?


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added!

@@ -0,0 +1,631 @@
{
Copy link
Copy Markdown
Collaborator

@gabrieltseng gabrieltseng May 26, 2025

Choose a reason for hiding this comment

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

This torchscripted model then only works for t=12 inputs right?

Thats fine for this use case but it might be clearer to have explicit variables (e.g. num_timesteps=12 ) here


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added

@@ -0,0 +1,631 @@
{
Copy link
Copy Markdown
Collaborator

@gabrieltseng gabrieltseng May 26, 2025

Choose a reason for hiding this comment

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

Can you add a bit describing why ClassifierHandler is necessary / what its doing?


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added short comment

'Embedding Generation Estimates\nCost: $' +
estimate(5.37) +
'-' +
estimate(10.14)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

what do the 5.37 and 10.14 represent?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

a: cost estimates per 1000km^2 based on real runs (which is why we divide by 1000 in line 27)

.add([-272.15, 0])
.divide([35, 0.03]);
}
//var ERA5_temp = ee.Image([0,0]).rename(ERA5_BANDS).clip(roi)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit - can be deleted

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

leaving in for experimentation

var elevation = ee.Image('USGS/SRTMGL1_003').clip(roi).select('elevation');
var slope = ee.Terrain.slope(elevation);
var SRTM_img = ee.Image.cat([elevation, slope]).toDouble().divide([2000, 50]);
//var SRTM_temp = ee.Image([0,0]).rename(SRTM_BANDS).clip(roi)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit - can be deleted

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

leaving in for experimentation


// 2. Cluster embeddings and display
var training = embeddings.sample({ region: roi, scale: 10, numPixels: 10000 });
var trainedClusterer = ee.Clusterer.wekaKMeans(7).train(training);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It might be adding a note here that 7 == number of clusters

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a small comment

Copy link
Copy Markdown
Collaborator

@gabrieltseng gabrieltseng left a comment

Choose a reason for hiding this comment

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

nice job Ivan !


var ENDPOINT =
'projects/presto-deployment/locations/us-central1/endpoints/vertex-pytorch-presto-endpoint';
var RUN_VERTEX_AI = true; // Leave this as false to get a cost estimate first
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can we this false by default

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes

@ivanzvonkov ivanzvonkov merged commit 59b1008 into main May 26, 2025
1 check passed
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