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

GPU Aggregation (8/8): Remove legacy aggregator #9100

Merged
merged 4 commits into from
Aug 29, 2024
Merged

Conversation

Pessimistress
Copy link
Collaborator

For #7457

For discussion: there is no more CPUGridLayer/GPUGridLayer, simply GridLayer. Should we keep the old layer exports with the gpuAggregation flag preset?

This affects the carto module @donmccurdy @felixpalmer

Change List

  • Move legacy AggregationLayer into the heatmap-layer directory. HeatmapLayer is the only remaining layer using it, and we are planning a rewrite.
  • Remove unused code and tests

Base automatically changed from x/aggregation-7 to master August 23, 2024 23:27
@Pessimistress Pessimistress marked this pull request as ready for review August 23, 2024 23:29
@coveralls
Copy link

coveralls commented Aug 23, 2024

Coverage Status

coverage: 90.933% (+1.6%) from 89.292%
when pulling b948340 on x/aggregation-8
into 0dd47b4 on master.

@donmccurdy
Copy link
Collaborator

If GPUGridLayer has never worked in the v9.x lifecycle then perhaps that isn't needed? But keeping CPUGridLayer as an alias of GridLayer would be a nice backward-compatibility approach I think.

@ibgreen
Copy link
Collaborator

ibgreen commented Aug 26, 2024

For discussion: there is no more CPUGridLayer/GPUGridLayer, simply GridLayer. Should we keep the old layer exports with the gpuAggregation flag preset?

I also think removing it seems reasonable. Probably not the most common layer, and easy enough for affected users to update.

@felixpalmer
Copy link
Collaborator

I think it is reasonable to remove the old layers, it is straightforward for users to rename the layer

@Pessimistress Pessimistress merged commit e3dde67 into master Aug 29, 2024
4 checks passed
@Pessimistress Pessimistress deleted the x/aggregation-8 branch August 29, 2024 17:12
@felixpalmer felixpalmer mentioned this pull request Sep 4, 2024
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.

5 participants