add layers into geospace

[wang-boyu]

This PR attempts to add Raster and Vector (GeoDataFrame) layers into GeoSpace. The component diagram looks like below:

classDiagram
  MapModuleJS ..> MapModule: visualises
  MapModule ..> GeoSpace: gets data
  GeoSpace *-- _AgentLayer
  GeoSpace *-- RasterLayer
  GeoSpace *-- GeoDataFrame
  _AgentLayer ..> GeoAgents: contains

Loading

Before it is ready to merge, the main issue is, there is a auto_convert_crs parameter in the GeoSpace.add_layer() method, similar to GeoSpace.add_agents(), which is really annoying. Image the users have sources of data with different crs from the GeoSpace. To avoid errors, they will have to:

space = GeoSpace()
space.add_layer(RasterLayer.from_file("some_file.tif"), auto_convert_crs=True)
space.add_layer(some_gdf, auto_convert_crs=True)
space.add_agents(some_agents, auto_convert_crs=True)

But we still need this parameter, due to the same reason as GeoAgents. Shall we do something like this instead -

space = GeoSpace(auto_convert_crs=True)
space.add_layer(RasterLayer.from_file("some_file.tif"))
space.add_layer(some_gdf)
space.add_agents(some_agents)

Or is there any other better way?

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@8e55655). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #67   +/-   ##
=========================================
  Coverage          ?   78.23%           
=========================================
  Files             ?        5           
  Lines             ?      363           
  Branches          ?        0           
=========================================
  Hits              ?      284           
  Misses            ?       79           
  Partials          ?        0           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e55655...96fc4d3. Read the comment docs.

[wang-boyu]

hmm let me look into the dependency issue in python 3.10

[Corvince]

space = GeoSpace(auto_convert_crs=True) looks pretty good to me! I missed taking part in the discussion about auto_convert_crs, but I think this is actually something that should happen by default. It is probably a good idea to have this option, but it should default to true, because that is what most people will want. But that is just my opinion, having it as an option on GeoSpace is fine by me.

[wang-boyu]

Thanks for the reply!

My concern with setting auto_convert_crs=True by default is that, I don't think it'll ever be set to False by users, and it gets ignored.

Instead, the point of having it to be False by default is, users get reminded if their data are of different crs. Subsequently they can choose to either a) manually change all crs to make them consistent, or b) set auto_convert_crs=True, knowing that crs conversion is done.

[rht]

For the auto_convert_crs decision, see the messages surrounding #58 (comment).

[Corvince]

Instead, the point of having it to be False by default is, users get reminded if their data are of different crs. Subsequently they can choose to either a) manually change all crs to make them consistent, or b) set auto_convert_crs=True, knowing that crs conversion is done.

That is exactly my point. There is no alternative to converting the crs, so why require the extra step of doing a) or b) ?
What is the user advantage here?

[wang-boyu]

Yes, there is no alternative to converting the crs to the same one. The question is, should the users be notified about any crs inconsistency and conversion?

If auto_convert_crs=True by default, everything could silently get converted to the default crs epsg:3857. For example:

space = GeoSpace()
space.add_layer(a_raster_layer_of_epsg_4326)
space.add_layer(a_geodataframe_of_epsg_4326)
space.add_agents(some_agents_of_epsg_4326)
# Surprise! Everything is in epsg:3857 now.

[Corvince]

Yes it shouldn't happen silently as a side effect, it should be a feature! That is, the function of add_agents is to add agents to the GeoSpace by converting their CRS to the crs of the GeoSpace. If we add to this to the docstring and also mention it again in the tutorial that should be sufficient information imho

[wang-boyu]

Very good! My apologies but I can't stop myself from thinking about this meme

Ok, so in summary, we have two options:

1. Remove the auto_convert_crs parameter. In docstrings and tutorial, we highlight to our users that, add_layer() and add_agents() will add layers and agents to GeoSpace by converting their CRS into that of the GeoSpace.
2. Put the auto_convert_crs parameter into GeoSpace constructor, and have it equal to False by default. Users get an ValueError whenever they attempt to add layers/agents of different CRS into GeoSpace (how dare you), unless they set auto_convert_crs=True.

@Corvince prefers Option 1 and I'm leaning towards Option 2.

@rht @tpike3 What do you think? Please feel free to come up with more options : )

[Corvince]

I am on vacation and mobile so I cannot verify this, but have you tested the original idea of raising just a warning? Because from how I understand it after reading about issuing warnings, they should actually only display once!? So the main concern about hundreds of warnings could be dismissed.

This could be option 3)

https://docs.python.domainunion.de/3/library/warnings.html#the-warnings-filter

[wang-boyu]

This is very interesting to know, and you're right! By default -

print the first occurrence of matching warnings for each location (module + line number) where the warning is issued

so there are not repeated warnings for GeoAgents. I tried it just now and it indeed worked like this.

In this case the parameter probably won't be named as auto_convert_crs, since we are converting crs automatically, and the parameter is to switch on/off the warning messages. (Pls correct me if I'm wrong).

Option 3: Always automatically convert layers/agents to the crs of GeoSpace if they're different. Have a warn_crs_conversion parameter in GeoSpace constructor, and have it equal to True by default. Users get a warning message when adding layers/agents of different CRS into GeoSpace, unless they set warn_crs_conversion=False.

Option 3 is my favorite now : )

[rht]

Voting for option 3. Didn't know that you could make it display only once.

[wang-boyu]

Implemented Option 3. Ready for review now.

[Corvince]

Great! Could you just make the warning option a kwarg? If we then add more options to GeoSpace later it won't be API breaking (without warning being always the second argument)

[rht]

In the last commit, I didn't see any code that would result in the warning being printed only once. It seems that users will have to manually do this:

warnings.simplefilter("once", ...)

This is not ideal.

There is another way to display the warning only once. Define an attribute self.warning_is_shown = False. If the warning is then later shown once, then set it to True, and never display the subsequent warnings.

[wang-boyu]

Let me try to explain this. For the following code (which is the default behavior):

1  for _ in range(10):
2      warnings.warn("This is a warning.")
3 
4  for _ in range(10):
5      warnings.warn("This is a warning.")

we will end up with two warnings instead of 20, one from line 2, the other one from line 5.

If we change from default to "once":

1  warnings.filterwarnings("once")
2
3  for _ in range(10):
4      warnings.warn("This is a warning.")
5
6  for _ in range(10):
7      warnings.warn("This is a warning.")

There is going to be only one warning, since the messages from line 4 and line 7 are the same.

In our case, I think the default behavior is good enough - one warning for hundreds of agents.

[wang-boyu]

Not sure if I've done it correctly, but I put warn_crs_conversion into kwargs. The examples are also updated accordingly.

[rht]

OK, that's fine then.

[wang-boyu]

Is there anything else that I should change for this PR...?

[rht]

Why is this **kwargs instead of just explicitly adding warn_crs_conversion?

[rht]

I think this comment means just a single keyword argument.

[wang-boyu]

@Corvince Would you like to clarify? I'm a bit confused whether it's a single keyword argument or kwargs here.

[wang-boyu]

I get it! warn_crs_conversion is now a keyword-only argument with default value of True.

[wang-boyu]

Is there any more changes that are needed in order to merge this PR?

[rht]

LGTM from my end. If @Corvince doesn't merge this within a day, I will do it.

[Corvince]

I don't have the time atm to do a proper review, so I'll just merge this with just a quick look.

Bit I am really excited about this one, because I was always quite clueless how to combine raster and vector data

[wang-boyu]

Thanks a lot for the help guys! Appreciate the comments.

I will keep working on raster layers for the next PR, and will need your feedback too : )