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

Adds overlapRasterGroupMetrics function #270

Merged
merged 2 commits into from
Mar 8, 2024
Merged

Adds overlapRasterGroupMetrics function #270

merged 2 commits into from
Mar 8, 2024

Conversation

avmey
Copy link
Contributor

@avmey avmey commented Jan 17, 2024

Functions calculating "grouped" metrics exist in the gp library for overlapFeatures and overlapArea, but not for raster overlap analysis.

Adds a corresponding overlapRasterGroupMetrics function to go along with the overlapAreaGroupMetrics and overlapFeaturesGroupMetrics functions, bringing this functionality from in-project code in the Belize reports.

Experimental release 6.1.3-experimental-rasterGroupMetrics.0 was tested in the Belize reports and confirmed working.

@avmey avmey linked an issue Jan 18, 2024 that may be closed by this pull request
Copy link
Contributor

@twelch twelch left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @avmey. I think it's a good addition. A couple of small changes requested.

Comment on lines 47 to 48
/** Raster to overlap, keyed by class ID, use empty array if overlapArea operation */
featuresByClass: Record<string, Georaster>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this extra part of comment be dropped? "use empty array if overlapArea operation". Maybe it was copied over from the other function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing that last part, it was an artifact as you expected

Comment on lines +178 to +180
featuresByClass:
| Record<string, Feature<Polygon>[]>
| Record<string, Georaster>;
Copy link
Contributor

@twelch twelch Mar 4, 2024

Choose a reason for hiding this comment

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

My understanding is that overlapGroupMetrics can now be called with either features or a raster. That seems to be the main reason to have mixed mixed vector and raster into the same typings? If so, would you be willing to write a test that this function can now handle either input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the case, yeah. I did keep the name the same and added the extra typing so there wouldn't be a breaking change. It's handling of raster input is tested in overlapGroupMetrics.test.ts, and I can add a test of the feature[] input.

@avmey avmey requested a review from twelch March 5, 2024 21:49
@avmey avmey merged commit d73e2d0 into dev Mar 8, 2024
2 checks passed
@avmey avmey deleted the rasterGroupMetrics branch March 8, 2024 21:09
twelch pushed a commit that referenced this pull request Jun 25, 2024
* Adds overlapRasterGroupMetrics function

* Tests overlapGroupMetrics can recieve feature[]
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.

Expand overlapGroupMetrics to accept rasters
2 participants