Conversation
…-based-queries-rebase
…-based-queries-rebase
…-based-queries-rebase
…-based-queries-rebase
…-based-queries-rebase
| impl Transformer<geodesy::ctx::Minimal> { | ||
| pub fn pro4_string_modifications(code: u16, proj4: &str) -> String { | ||
| match (code, proj4) { | ||
| (3857, _) => proj4.replace("merc", "webmerc"), |
There was a problem hiding this comment.
guess this should be documented
There was a problem hiding this comment.
also: are there any other cases that require this?
would it make sense to curate this in the crs definitions crate?
There was a problem hiding this comment.
yes. This is a very complex topic. I do not know if other projections need this. Also the crs_definitions crate is not bound to Geodesy and as i see it this is something only geodesy requires. We should upstream this to the geo-geodesy crate but there are other things that needs to be done there first.
| }); | ||
| } | ||
|
|
||
| if source.code() > u32::from(u16::MAX) { |
There was a problem hiding this comment.
isnt GOOGLE crs larger than u16?
There was a problem hiding this comment.
yes but it is not a real EPSG code. This would need something handling "alias" names
| } | ||
| } | ||
|
|
||
| impl<C: geodesy::ctx::Context> Transformer<C> { |
There was a problem hiding this comment.
are there even multiple geodesy context implementations?
There was a problem hiding this comment.
yes there are two and they plan a "ParallelContext" upstream
| }; | ||
|
|
||
| pub enum MixedCoordinateProjector { | ||
| ProjProjector(super::proj_projector::ProjCoordinateProjector), |
There was a problem hiding this comment.
unrelated to this line: in general we need to think about whether the approach to use geodesy because of performance and fall back to proj only when geodesy does not support a projection suits all our use cases. can we be reasonably certain that we will not need some crs in the future that geodesy does not support and where we need good performance? because then we would need to implement proj context reusage anyway.
There was a problem hiding this comment.
well this is a very interesting question. One "projection" not implemented in Godesy is the GEOS projection. However, we could create a PR if we need it (it is not very complex). Basically all non-EPSG projections are not supported but this is not really Geodesys fault. Geodesy should add a WKT parser... Currently it requires the proj4 strings and we get them from crs-definitions and there are only EPSG codes listed.
There was a problem hiding this comment.
currently, all projections we use should work. However, if someone uses DHDN then we have a problem.
| pub struct Transformer<C: geodesy::ctx::Context = geodesy::ctx::Minimal> { | ||
| ctx: C, | ||
| source_crs: u16, | ||
| source: geodesy::ctx::OpHandle, |
There was a problem hiding this comment.
why store the ophandle?
There was a problem hiding this comment.
this is what you use to call the "fwd" and "rew" projection
| ctx: C, | ||
| source_crs: u16, | ||
| source: geodesy::ctx::OpHandle, | ||
| source_latlon: bool, |
There was a problem hiding this comment.
is this not derivable fro the source/target crs?
There was a problem hiding this comment.
well i derive it by looking for "latlong" in the proj4 string. I do not think there is a better way at the moment. Since we use this for every coordinate we should remember this instead of the proj string. However, if you see a better way please let me know :)
| #[derive(Debug, Clone)] | ||
| pub struct Transformer<C: geodesy::ctx::Context = geodesy::ctx::Minimal> { | ||
| ctx: C, | ||
| source_crs: u16, |
There was a problem hiding this comment.
is this always epsg code? are all relevant projections describable by this?
There was a problem hiding this comment.
Geodesy currently parses the proj4 strings to create objects. We need to get the proj4 strings somewhere and the easiest way is to get them from the crs-definitions crate (this is what geo-geodesy does). The crs-definitions crate only has EPSG codes at the moment. We would need to add more e.g. ESRI if we need them
| let source_latlon = source_def.proj4.starts_with("+proj=longlat"); | ||
| let target_latlon = target_def.proj4.starts_with("+proj=longlat"); | ||
|
|
||
| let source_proj_string = Self::pro4_string_modifications(source_crs, source_def.proj4); |
There was a problem hiding this comment.
can't we store the definitions "correctly" in the first place instead of modifying them each time?
There was a problem hiding this comment.
of the crs-definitions should be useable by other crates then no. If we want them only for geodesy then yes.
|
|
||
| pub fn transform_coord(&self, coord: Coordinate2D) -> Result<Coordinate2D, Error> { | ||
| let mut geodesy_coord = if self.source_latlon { | ||
| [geodesy::coord::Coor2D::gis(coord.x, coord.y)] |
There was a problem hiding this comment.
so we need to copy all data. does proj do this too?
There was a problem hiding this comment.
i think we can also implement a trait. We need to look that up.
| [geodesy::coord::Coor2D::raw(coord.x, coord.y)] | ||
| }; | ||
| self.ctx | ||
| .apply(self.source, geodesy::Direction::Inv, &mut geodesy_coord)?; |
There was a problem hiding this comment.
why does geodesy look up the op in the context each time? can we prevent this? we always use the same op but it does a registry lookup each time
There was a problem hiding this comment.
it is a very strange design. We would need to look into this.
| */ | ||
| } | ||
|
|
||
| pub fn geodesy_ctx() -> geodesy::ctx::Minimal { |
There was a problem hiding this comment.
thie Minimal context is also not Send or Sync or is it?
There was a problem hiding this comment.
nope but it is fast. There is an issue for a ParallelContext. Maybe this would be Send or Sync.
| }; | ||
|
|
||
| pub struct ProjCoordinateProjector { | ||
| pub from: SpatialReference, |
There was a problem hiding this comment.
can this not be derived from the Proj instance?
There was a problem hiding this comment.
not in a way that would be halpfull
| ) -> Result<SpatialGridDescriptor> { | ||
| let proj_from_to = | ||
| CoordinateProjector::from_known_srs(in_srs, params.target_spatial_reference)?; | ||
| MixedCoordinateProjector::from_known_srs(in_srs, params.target_spatial_reference)?; |
There was a problem hiding this comment.
wouldn't it still make sense to try to reuse the projector? did you measure the overhead of instantiation?
There was a problem hiding this comment.
i created a small benchmark. It is much faster then proj.. You can add more benches if you have ideas how to measure that.
…longer works now that resolutions are no longer part of the query rectangle
- [ ] I added an entry to [`CHANGELOG.md`](CHANGELOG.md) if knowledge of this change could be valuable to users. --- Here is a brief summary of what I did: <TEXT>
…ks now that resolutions are no longer part of the query rectangle (#1112) - [ ] I added an entry to [`CHANGELOG.md`](CHANGELOG.md) if knowledge of this change could be valuable to users. --- Here is a brief summary of what I did: <TEXT>
…e/geoengine into new-pixel-based-queries-rebase
…e/geoengine into proj_experiments
…e/geoengine into proj_experiments
- [ ] I added an entry to [`CHANGELOG.md`](CHANGELOG.md) if knowledge of this change could be valuable to users. --- Here is a brief summary of what I did: <TEXT>
…e/geoengine into proj_experiments
| ) -> Result<Vec<Coordinate2D>> { | ||
| let c_ref = coords.as_ref(); | ||
|
|
||
| let mut cc = Vec::from(c_ref); |
Check failure
Code scanning / CodeQL
Cleartext logging of sensitive information High
Copilot Autofix
AI about 1 month ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
CHANGELOG.mdif knowledge of this change could be valuable to users.Here is a brief summary of what I did: