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

Don't set crs in attrs. #71

Open
dcherian opened this issue Jul 2, 2024 · 5 comments
Open

Don't set crs in attrs. #71

dcherian opened this issue Jul 2, 2024 · 5 comments

Comments

@dcherian
Copy link
Contributor

dcherian commented Jul 2, 2024

Currently the crs is duplicated in attrs and on the index.

The edge-case to handle is when you select with a scalar the index is dropped. In this case we may want to set it on the variable attrs in IndexSelResult.

This commit is a good starting point.

@dcherian dcherian changed the title Don;t set crs in attrs. Don't set crs in attrs. Jul 2, 2024
@martinfleis
Copy link
Member

I think I know the reason we originally did this. When selecting via a scalar coordinate from GeometryIndex, the result no longer has an index along the dimension, hence loses information on CRS. So when using the traffic_dataset from our tests

ds = traffic_dataset.sel(
    origin=shapely.Point(1, 2), day="2023-01-01", mode="car"
)

ds no longer contains any information on CRS of origin if we don't set attrs["CRS"]. I am not sure if it is a good enough reason to keep assigning it. In our tests this pops out in conversion to a GeoDataFrame, where resulting origin GeoSeries does not have a CRS assigned.

@dcherian
Copy link
Contributor Author

Yes I think this is where the CRSIndex idea is useful. It can always be tied to a scalar spatial_ref variable and propagated.

@benbovy
Copy link
Member

benbovy commented Dec 4, 2024

this is where the CRSIndex idea is useful. It can always be tied to a scalar spatial_ref variable and propagated.

That is a great idea! We've been working on that with @scottyhq, the result is here: https://github.com/benbovy/xproj 🙂 (still experimental and very much work in progress).

@dcherian
Copy link
Contributor Author

dcherian commented Dec 4, 2024

Could've called it crx... ;) exciting to see nevertheless

@benbovy
Copy link
Member

benbovy commented Dec 4, 2024

Ah yes crx that sounds nice I like it! I guess we've been a little more pragmatic and chose the boring one :).

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

No branches or pull requests

3 participants