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

Simple and consistent settings for coordinate reference system (CRS) #49

Closed
wang-boyu opened this issue Apr 3, 2022 · 7 comments
Closed

Comments

@wang-boyu
Copy link
Member

wang-boyu commented Apr 3, 2022

What's the problem this feature will solve?

Consistent coordinate reference system (CRS) is important for map processing and visualisation. Incorrect CRS settings can lead to incorrect or empty map in mesa-geo.

Currently the workflow is:

flowchart TD
    A["Shapefiles<br><br>arbitrary CRS<br>(might be missing)"] --> |read by| B["GeoPandas"];
    B --> |create| C["GeoDataFrame<br><br>same CRS as shapefiles<br>(might be missing)"]
    C -->|loaded by| D["AgentCreator<br><br>user-defined CRS<br>(default: epsg:3857)"];
    E["GeoJSON<br><br>assuming no CRS"] --> |loaded by| F["GeoPandas"] --> |create| G["GeoDataFrame<br><br>hard-coded CRS<br>(set to epsg:4326)"] --> |loaded by| D;
    D --> |create| H["GeoAgent<br><br>no explicit CRS<br>implicitly same as AgentCreator<br>(default: epsg:3857)"]
    H --> |added into| I["GeoSpace<br><br>user-defined CRS<br>(default: epsg:3857)"]
    I --> |rendered by| J["MapModule<br><br>hard-coded CRS in GeoSpace<br>(set to epsg:4326)"]
    J --> |displayed by| K["LeafletMap.js<br><br>expects input as epsg:4326<br>internally transforms into epsg:3857"]
Loading

The following issues should be fixed:

  1. CRS is an optional field in GeoJSON. When it's present, it should be loaded correctly instead of overwritten by the default epsg:4326.
  2. The CRS of frontend element (e.g., LeafletMap.js) should be inside its corresponding Python module (e.g., MapModule.py). Similarly the coordinate transformation from GeoSpace to map elements should be done inside the map elements.
  3. Account for empty CRS if it's not available from input files or GeoJSON. This relates to Option to turn on/off basemap #48.

Describe the solution you'd like

The solution should make sure the following constraints are met:

  1. AgentCreator, GeoAgent and GeoSpace have exactly the same CRS. At the moment they share the same defaults, but there's a chance for users to set them into different CRS, thereby resulting in incorrect map.
  2. MapModule has the same CRS as the frontend JavaScript map element. Right now the default CRS of Leaflet.js is set correctly, but inside GeoSpace instead of MapModule, making it difficult to use different frontend map elements of other CRS.

Additional context

@wang-boyu
Copy link
Member Author

I just noticed that epsg:4326 is the CRS for all GeoJSON: https://datatracker.ietf.org/doc/html/rfc7946#section-4

@Corvince
Copy link
Contributor

I just noticed that epsg:4326 is the CRS for all GeoJSON: https://datatracker.ietf.org/doc/html/rfc7946#section-4

Yes, this was the reason I originally hardcoded it to epsg:4326. However, from re-reading the spec and looking around on the internet this is a bit unclear. I mean if there is no crs attribute it is correct to use epsg:4326, but if it is set we should respect it. Currently GeoDataFram.from_features is used, which does not seem to check the crs, but I am not sure

@Corvince
Copy link
Contributor

BTW I really like your diagram!

But I think this boils a lot down to #48.

I might totally misrember that, but I think currently the CRS of the GeoSpace is only used for setting up the transformation inside MapModule. So it should probably removed from GeoSpace and the transformation inside MapModule (which should be renamed to LeafletMapModule) should transform from the agents CRS to epsg:4326

@wang-boyu
Copy link
Member Author

wang-boyu commented Apr 13, 2022

Thanks a lot for your comments! I really need some opinions on how to design the GIS functionalities moving forward.

Yes, this was the reason I originally hardcoded it to epsg:4326. However, from re-reading the spec and looking around on the internet this is a bit unclear. I mean if there is no crs attribute it is correct to use epsg:4326, but if it is set we should respect it. Currently GeoDataFram.from_features is used, which does not seem to check the crs, but I am not sure

After reading more about GeoJSON, I am in fact thinking to honor the epsg:4326 gcs in its spec, as this might be the case in most other python packages. Thereafter we can use GeoJSON as the interfacing protocol between backend python modules and frontend js elements, much like what you were already doing, but probably with a few small changes.

I might totally misrember that, but I think currently the CRS of the GeoSpace is only used for setting up the transformation inside MapModule.

Yup this is correct.

So it should probably removed from GeoSpace and the transformation inside MapModule (which should be renamed to LeafletMapModule) should transform from the agents CRS to epsg:4326

If like I mentioned above, GeoJSON is still going to be used as the interface between backend & frontend, then the transformation of GeoAgent and GeoSpace into epsg:4326 can be done in their own __geo_interface__ functions respectively.

I am considering adding a crs attribute for GeoAgent objects, so that they can transform by themselves. When being added into GeoSpace, they will be transformed into the crs of GeoSpace. In this way we don't need the implicit constraint that AgentCreator and GeoSpace should have the same crs.

This is one of the things I'm working on at the moment. Will add you into the PR for your feedback when it's ready.

@Corvince
Copy link
Contributor

After reading more about GeoJSON, I am in fact thinking to honor the epsg:4326 gcs in its spec, as this might be the case in most other python packages. Thereafter we can use GeoJSON as the interfacing protocol between backend python modules and frontend js elements, much like what you were already doing, but probably with a few small changes.

For writing GeoJSON we should definitely stick to epsg:4326, for reading (if crs present) - I have no idea how this is usually handled. But yeah, this was the originally idea of the geo_interface

If like I mentioned above, GeoJSON is still going to be used as the interface between backend & frontend, then the transformation of GeoAgent and GeoSpace into epsg:4326 can be done in their own __geo_interface__ functions respectively.

True.

I am considering adding a crs attribute for GeoAgent objects, so that they can transform by themselves. When being added into GeoSpace, they will be transformed into the crs of GeoSpace. In this way we don't need the implicit constraint that AgentCreator and GeoSpace should have the same crs.

That sounds like a sane approach.

This is one of the things I'm working on at the moment. Will add you into the PR for your feedback when it's ready.

Great! Really happy to see work being done on mesa-geo!

@wang-boyu
Copy link
Member Author

Happy to hear some feedback from you too! We should have more conversations in the future : )

@wang-boyu wang-boyu mentioned this issue Apr 15, 2022
@wang-boyu
Copy link
Member Author

Closed via #58.

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

2 participants