Skip to content

feat: Add ml model input and output shape to allow models run on entire tiles #1000

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

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

Conversation

jdroenner
Copy link
Member

  • I added an entry to CHANGELOG.md if knowledge of this change could be valuable to users.

Here is a brief summary of what I did:

@coveralls
Copy link
Collaborator

coveralls commented Nov 26, 2024

Pull Request Test Coverage Report for Build 13195982647

Details

  • 109 of 194 (56.19%) changed or added relevant lines in 10 files are covered.
  • 3 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.04%) to 90.031%

Changes Missing Coverage Covered Lines Changed/Added Lines %
services/src/machine_learning/mod.rs 0 2 0.0%
operators/src/machine_learning/metadata_from_file.rs 14 27 51.85%
services/src/api/model/datatypes.rs 10 24 41.67%
datatypes/src/machine_learning.rs 12 34 35.29%
operators/src/machine_learning/onnx.rs 28 62 45.16%
Files with Coverage Reduction New Missed Lines %
services/src/contexts/postgres.rs 1 96.9%
operators/src/machine_learning/metadata_from_file.rs 1 52.86%
services/src/api/handlers/tasks.rs 1 99.53%
Totals Coverage Status
Change from base Build 13173496179: -0.04%
Covered Lines: 126035
Relevant Lines: 139990

💛 - Coveralls

@jdroenner jdroenner changed the title add tensor shape to ml model feat: Add ml model input and output shape to allow models run on entire tiles May 19, 2025
);
let session = load_onnx_model_from_metadata(&self.model_metadata)?;

// TODO: check @ initialize?
Copy link
Contributor

@michaelmattig michaelmattig May 20, 2025

Choose a reason for hiding this comment

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

or: check when adding the model to Geo Engine? model fitting to the metadata is independent of the query

.run(ort::inputs![input_name => samples].context(Ort)?)
.context(Ort)?;
let outputs = if self.model_metadata.input_is_single_pixel() {
let rows = width * height;
Copy link
Contributor

Choose a reason for hiding this comment

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

use pixels.len()?

Ok(out)
} else if self.model_metadata.input_shape.yx_matches_tile_shape(&tile_shape){
let samples = Array4::from_shape_vec((1, height, width, num_bands), pixels).expect( // y,x, attributes
"Array2 should be valid because it is created from a Vec with the correct size",
Copy link
Contributor

Choose a reason for hiding this comment

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

Array4?

"Array2 should be valid because it is created from a Vec with the correct size",
);

let input_name = &session.inputs[0].name;
Copy link
Contributor

Choose a reason for hiding this comment

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

the code starting here is the same for both cases, maybe refactor it to occur only once

Copy link
Member Author

Choose a reason for hiding this comment

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

samples has a different type so i can not move it into one case. I can move it into a method but this will not be less code...


Ok(out)
} else {
Err(
Copy link
Contributor

Choose a reason for hiding this comment

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

can this case even happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

it should not but if it happens i guess an error is better then paniic?

@@ -347,7 +374,62 @@ mod tests {
};
use ndarray::{Array1, Array2, arr2, array};

use super::*;
fn generate_model_metadata_from_onnx(
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this method itself be tested to generate the correct metadata for the individual onnx files?

@@ -327,11 +346,19 @@ impl_no_data_value_zero!(i8, u8, i16, u16, i32, u32, i64, u64);

#[cfg(test)]
mod tests {
use std::path::Path;
Copy link
Contributor

@michaelmattig michaelmattig May 20, 2025

Choose a reason for hiding this comment

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

unrelated to this line: add a test with a 3 model?

Ok(())
}

pub fn try_onnx_tensor_to_ml_tensorshape_3d(
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe test this method

exe_context.tiling_specification().tile_size_in_pixels,
)?;
// initialize model
// TODO: re-use session accross queries?
Copy link
Contributor

@michaelmattig michaelmattig May 20, 2025

Choose a reason for hiding this comment

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

there is no query happening? comment copy pasted?

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.

3 participants