Skip to content

Conversation

@MartinSJRogers
Copy link
Collaborator

This PR builds upon #19 by retaining the patchwise predictions as deepsensor.prediction objects. This reduces the indentation and number of for loops within stitch_clipped_predictions(). Because the code uses the .where() function to merge patchwise predictions, there is still a necessity to generate a blank prediction object with the same extent, keys and data variables at the entire extent of X_t. There may be a shorter way to do this using prediction.py but I could not see this.

Copy link
Owner

@davidwyld davidwyld left a comment

Choose a reason for hiding this comment

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

Note: I've just tried to review the last commit (58e9076) because the others seem to have been on the branch merged in #19, I'm not sure why the commits are on both branches.

Seems to do the trick! Nice work!

) - patch_overlap[0]
else:
b_x1_max = b_x1_max
first_key, first_value = next(iter(patch_pred.items()))
Copy link
Owner

Choose a reason for hiding this comment

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

I'm a little wary of this line, is this getting the first item in patch_pred? If so, I think that using next(iter( is a bit clunky, but also because dictionaries aren't guaranteed to be ordered consistently. Could we do this with a particular key? first_value isn't used so it seems like we only need the key here.

Copy link
Owner

Choose a reason for hiding this comment

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

Looking more into this, apparently dictionaries are ordered for python 3.7+ so we're safe. But it might be safer to use something like list(patch_pred.keys)[0] if we only need the key.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Happy to change it to this. There's always many ways to skin a cat, but I agree your way seems safer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason for doing this is just because I only need one variable name to work out things like coordinates and extent. So actually it shouldn't matter in this instance if the variable names are different, but good to ensure its consistent just in case.

@davidwyld davidwyld merged commit 1f0fb32 into patchwise_train Jan 27, 2025
4 of 10 checks passed
@davidwyld davidwyld deleted the simplify_stitching_retain_prediction_objects branch January 27, 2025 11:05
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