Large area simulation rework

[KieranRatcliffeInvertedAI]

Moved versions of initialize and drive designed for simulating on large maps to a different submodule called "large" in invertedai.

• Developed a new basic datatype called Region which contains a center, fov, and agent information such as state, attributes, and recurrent state as well as helper functions. This data type was designed with some thought towards the future in case basic assumptions about what a "region" can be changed (e.g. bigger or smaller than 100x100m square, non-square regions).

• Due to the lack of need for the pygame visualization and design constraint of being stateless, region_drive has become simpler and less reliant on dependencies.

• Region initialize is now separated from HOW the regions are produced. Default helper functions have been provided for this but can be easily swapped out by the user.

• The core functionality is largely the same as it was before, algorithmically speaking.

Questions for reviewers:

• What should we do concerning support for old versions of these utilities?
• Do the comments make sense? Should more be added?
• Any other thoughts on the structure of the interface or future-proofing concerns?

[Ruishenl]

Some initial comments

[Ruishenl]

If you are referring to vertex, i believe the plural form is vertices.

[KieranRatcliffeInvertedAI]

Vertices is indeed used but Vertexes is also used and more grammatically correct.

[Ruishenl]

Are we sure we want to keep using the word Particle here ? TreeNode or Node make more sense to me here. Or we can simply just call it Agent in this local namespace.

[KieranRatcliffeInvertedAI]

This was a carry over from the old area drive, however particle is the most commonly used term for "things that are placed within a quad tree". However Agent would indeed be more effective and descriptive.

[Ruishenl]

It makes sense to have a doc string for this method explaining how things work in general, but you should reformat it to have it enclosed instead of using the comments.
And i don't think you need those additional comments explain how things work line by line

[KieranRatcliffeInvertedAI]

This function was removed since it is not needed at this time.

[adscib]

Here's some feedback for the parts I managed to read tonight. I'll add more tomorrow. Generally, the structure is looking good and this is what we want, but there are a lot of things that can be simplified and renamed to something more obvious.

If we're confident that the old versions of those functions are not used, I think it's fine to delete them. If we thought they might be used, deprecating them for some time would be in order.

One name change to consider is to use large_initialize and large_drive instead of region_initialize and region_drive. Those are more in line with what those functions accomplish rather than how they do it.

[adscib]

Suggested change

	A data structure to contain information about a region in a map to be used in large simulations.
	A region in a map used to divide a large simulation into smaller parts.

It is self-evidence that this is a data structure

[KieranRatcliffeInvertedAI]

Done.

[adscib]

Suggested change

	center: Optional[Point] = None # The center of the region if such a concept is relevant (e.g. center of a square, center of a rectangle)
	center: Optional[Point] = None #: The center of the region if such a concept is relevant (e.g. center of a square, center of a rectangle)

Use #: for each field to include those comments in the documentation.

[KieranRatcliffeInvertedAI]

Done.

[adscib]

size or length would be a better name, as the region definition doesn't necessarily have anything to do with viewing.

[KieranRatcliffeInvertedAI]

Done.

[adscib]

Is there a semantic difference between None and [] here? I'm also wondering if we should make it a List[Tuple[AgentAttributes, AgentState, Optional[RecurrentState]]] instead, but I'm not convinced.

[KieranRatcliffeInvertedAI]

If these Regions were only used for driving then I would agree with the Tuple but since it's used for initialize as well which can be quite wonky, I'd say lets keep state, attributes, and recurrent states separate.

[adscib]

Needs a docstring explaining how the grid is produced and what is a typical use case.

[KieranRatcliffeInvertedAI]

Done.

[adscib]

BFS is way of an overkill here. You can compute the grid of a rectangular area in closed form.

[KieranRatcliffeInvertedAI]

For the sake of time, I can update this later since this won't impact the user interface and we know this code works so we can quietly make that change later while users are testing the features?

[adscib]

Yes, it can be done later.

[adscib]

These argument names, as well as the name of the function itself, are not at all intuitive. Presumably what this function does is compute agent numbers for each region given some parameters, so the naming should reflect that. Neither density nor scaling have obvious meanings here.

[KieranRatcliffeInvertedAI]

I've renamed them but when could you provide some feedback on if the new names are insufficient, too wordy, and if so, could you elaborate on what you are looking for?

[adscib]

Suggested change

iterable_regions = None

[KieranRatcliffeInvertedAI]

Deleted.

[adscib]

Suggested change

	# Convert to grayscale using Luminosity Method: gray = 0.114*B + 0.587*G + 0.299*R
	# Image should be in BGR color pixel format
	birdview_grayscale = np.matmul(birdview.reshape(total_num_pixels,3),np.array([[0.114],[0.587],[0.299]]))
	number_of_black_pix = np.sum(birdview_grayscale == 0)
	number_of_black_pix = np.sum((birdview.sum(axis=-1) == 0).int())

This is a very elaborate conversion only to compare things to zero at the end.

[KieranRatcliffeInvertedAI]

Hm yeah I guess at this point we know we don't need to do anything more fancy with a grayscale image. I made this change.

[rf-ivtdai]

Generally I see 3 types of comments:

1. definition to arguments and fields
2. explanation of key steps within a long workflow
3. explanation of why things are called/implemented as such

For this repo, I believe type 1 and 2 are reasonable, but we should remove/rephrase those that are type 3. I marked them below.

[rf-ivtdai]

remove?

[KieranRatcliffeInvertedAI]

Done.

[rf-ivtdai]

Consider removing the comment

[KieranRatcliffeInvertedAI]

Done.

[rf-ivtdai]

Suggested change

	if len(region.agent_states) == 0:
	# Region is empty, do not call DRIVE
	continue
	else:
	if len(region.agent_states) > 0:

[KieranRatcliffeInvertedAI]

Done.

[adscib]

Here's a more careful review of initialize. I'll review drive as well, but posting this already in the interest of expediency.

[adscib]

This is a little tricky, but for the traffic lights to work correctly, you need to return the recurrent light states from the response that originally set them. When you provide light states as input, the recurrent light states returned are garbage. @rf-ivtdai is adding this explanation to the documentation, but you'll want to fix the returned value here.

[KieranRatcliffeInvertedAI]

I see. I've made a change that should account for this.

[adscib]

It's wasteful to request a birdview on every call, if you're only using the last one. I would remove this option altogether, but if you keep it, maybe there's a way to only request one birdview?

[KieranRatcliffeInvertedAI]

I'm fine with leaving this out for now because it's not-trivial how to interpret this in the "large" version of initialize and drive and there are indeed other visualization methods.

[adscib]

It's a little odd to just ignore this. This breaks the usual contract of INITIALIZE, which is that you get exactly as many agents as you request, or it's a failure. Let's discuss in person.

[adscib]

Yes, it can be done later.

[adscib]

I think you can pull this out of the loop?

[KieranRatcliffeInvertedAI]

The regions are populated with agents one at a time. Therefore, each loop all agents currently in the simulation (initialized, preddefined, whatever) need to be accessed so that ones that are relevant to the current region of interest can be included as conditional. An alternative would be to have 3 running lists for states, attributes, and recurrent states but I went with this method for the sake of saving memory space and code cleanliness at the cost of a small increase in computation time due to looping through every region.

[adscib]

Suggested change

	agent_attributes=[] if len(all_agent_attributes) == 0 else all_agent_attributes,
	agent_attributes=all_agent_attributes,

[KieranRatcliffeInvertedAI]

Done

[adscib]

Aren't those already captured in conditional_agent_states? It looks like you're including them twice.

[KieranRatcliffeInvertedAI]

The difference here is that conditional_agent_states contains states from regions that have already been initialized and predefined agents from other regions whereas region_predefined_agent_states contains predefined agents for the current region of interest. Any of these agents could be relevant as "conditional" for calling initialize on the current region.

[KieranRatcliffeInvertedAI]

I've renamed conditional_agent_states to out_of_region_conditional_agent_states and used this naming scheme wherever else relevant.

[adscib]

This is another instance where we remove some agents that the user is expecting to see. Perhaps this is fine, but we need to be clear in the documentation about what we return.

[KieranRatcliffeInvertedAI]

Actually upon further inspection, this can be removed because of the filter earlier in the loop that only takes in agents as conditional if they are within a certain scope so no collisions should happen at the edge anyways.

[adscib]

Modifying Region objects in place is another thing that we need to be careful about in the documentation.

[KieranRatcliffeInvertedAI]

Fixed so that it uses the member function region[i].insert_all_agent_details instead of modifying the fields directly.

[adscib]

Are you sure this is correct? I don't know what exactly was the intention here, but this seems suspicious to me.

[KieranRatcliffeInvertedAI]

My current understanding of initialize is that the order of agents matters, insomuch as if N is the number of conditional agents each with an AgentState in the the states_history argument, then initialize only samples M-N agents where M is the number of AgentAttributes in the agent_attributes argument. That is fine and dandy however the trickiness here is that there are two types of "conditional" agents here: conditional agents predefined within the region of interest and conditional agents that exist outside of the region of interest. The former need to be kept within the region whereas the former, at this point in the loop have served their purpose and can be ignored. So this slice is trying to collect just the conditional agents predefined within this region.

[KieranRatcliffeInvertedAI]

However, now with the deletion of that "valid states" filter, this is moot and the slice can just be len(out_of_region_conditional_agent_states) onwards.

[adscib]

Here's a review for drive. I'll do one more read of the whole thing.

[adscib]

Suggested change

	capacity: Optional[int] = 100,
	single_call_agent_limit: Optional[int] = None,

Setting the default to None will make for a seamless transition if we increase the agent limit in DRIVE in the future.

[KieranRatcliffeInvertedAI]

This makes sense, change is implemented.

[adscib]

Suggested change

	is_async: Optional[bool] = True
	async_api_calls: bool = True

[KieranRatcliffeInvertedAI]

Done

[adscib]

Maybe check that all the input lists are the same length? I believe that's required here.

[KieranRatcliffeInvertedAI]

Done.

[adscib]

Suggested change

	agent_x = [None]*num_agents
	agent_y = [None]*num_agents
	agent_x = [None for _ in range(num_agents)]
	agent_y = [None for _ in range(num_agents)]

What you did works, but I recommend this as a habit. Otherwise you end up getting surprising bugs when you change None to a mutable object, and you get N references instead of N separate objects.

[KieranRatcliffeInvertedAI]

Done.

[adscib]

This would be the idiomatic Python equivalent.

Suggested change

	for i, agent in enumerate(agent_states):
	agent_x[i] = agent.center.x
	agent_y[i] = agent.center.y
	agent_x = [agent.center.x for agent in agent_states]
	agent_y = [agent.center.y for agent in agent_states]

[KieranRatcliffeInvertedAI]

Done

[adscib]

Under what circumstances would we ever get no responses? Only if there are no input agents? In that case, I think we'd want behavior consistent with that drive does with no agents - easiest to achieve by making a single call to drive and returning the response.

[KieranRatcliffeInvertedAI]

Good point, I've made that change as well as adding an if statement to check whether it should just call regular drive (i.e. if there is only 1 leaf node).

[KieranRatcliffeInvertedAI]

If the user happens to input no agent information, the regular call to drive() will handle that.

[adscib]

Refactor repeated parts, len(region.agent_attributes) and zip(all_responses,non_empty_regions) into named variables for better readability.

[KieranRatcliffeInvertedAI]

I put the zipped lists into a single variable and added a "get number of agents" function to the quad tree leaf node to make it more readable.

[KieranRatcliffeInvertedAI]

Actually scratch that, if you attempt to refactor the zip part into a single variable, I think there is some pass-by-reference shenanigans where that zipped variable is being affected by the _flatten_and_sort function and that's why I had to repeat it before.

[adscib]

Suggested change

	flat_list = []
	for sublist in nested_list:
	flat_list.extend(sublist)
	[x for sublist in nested_list for x in sublist]

[KieranRatcliffeInvertedAI]

Done.

[adscib]

Suggested change

	sorted_list = []
	for ind in range(len(index_list)):
	agent_index = index_list.index(ind)
	sorted_list.append(flat_list[agent_index])
	sorted_list = [x[1] for x in sorted(zip(index_list, flat_list))]

[KieranRatcliffeInvertedAI]

Done.

[adscib]

This whole _flatten_and_sort business is quite convoluted and could use refactoring, but we can leave that for later.

[KieranRatcliffeInvertedAI]

It is an annoying circumstance for sure. There is no connection between the location of the agent and it's index in the input parameter. This is a situation where an explicit ID for each agent would be useful but I understand it wouldn't make sense to change one of our basic data classes just to solve this.

I think a middle ground is that this flatten and sort method can be a member function of the quadtree class instead to make it a bit cleaner remaining functionally the same.

[KieranRatcliffeInvertedAI]

Outstanding issues:

1. Despite some small buffer room from rounding up the region_size for the root layer of the quadtree, some gaps still exist between leaf nodes due to floating point issues when dividing the size of the leaf nodes over and over.
2. I've only seen this once but I got a recursion limit error, perhaps because there was one region on the map that was too dense and and, because the neighbouring regions were also dense, could not subdivide to a size of leaf node that contained less than the capacity.

[adscib]

Nice, it looks like we have the major things covered. Other than for some final tidying up, we should be good to go. Switching to an iterative QuadTree implementation would be my recommendation for getting rid of the recursion depth errors, but that can wait for later. More importantly, we need some test to ensure those new functions won't get broken by subsequent commits, but that can also be done in a separate PR.

[adscib]

Why are these optional? It would seem that both are needed to define a region.

[KieranRatcliffeInvertedAI]

Ah yes a oversight when this was changed from the more general Region, this has been updated.

[adscib]

Suggested change

	recurrent_states: Optional[List[RecurrentState]] = [] #: Recurrent states of the agents contained within the region
	recurrent_states: Optional[List[RecurrentState]] = [] #: Recurrent states of the existing agents within the region

Only if I understand correctly that this is supposed to have the same length as agent_states if given.

[KieranRatcliffeInvertedAI]

Correct, if recurrent_states is not empty, it should be the same length as agent_states AND agent_attributes. The two uses cases that I can identify for this object are for passing as arguments to DRIVE and to INITIALIZE which have slightly different needs so it's hard to enforce a standard across the board, especially because INITIALIZE has many different operating modes (i.e. equal number states and attributes, more attributes than states, no states at all).

Do you think it'd be worthwhile to split these regions up? Or have some sort of data validation? E.g. if agent details are to be added, it should check that it is a valid configuration?

[adscib]

Yes, I would perform some validation.

[adscib]

Suggested change

	def init_square_region(
	def create_square_region(

init may be confused with initialize

[KieranRatcliffeInvertedAI]

Done.

[adscib]

Suggested change

	"""
	Helper function to clear all agent data within the Region but keep geometric information.
	"""

I don't think this sentence add any information beyond what the name already says, so I would remove it to reduce the maintenance burden.

[KieranRatcliffeInvertedAI]

Done.

[adscib]

Suggested change

	"""
	Helper function to insert the details relevant to a single agent.
	"""

I don't think this one is even correct. Case in point that it's best to avoid docstrings that don't provide any extra information.

[KieranRatcliffeInvertedAI]

Done.

[adscib]

You're returning a list of regions. Make sure to specify whether those are the input Region objects, and if not whether the input objects are modified.

[KieranRatcliffeInvertedAI]

Done.

[adscib]

A copy method would be a good idea for future proofing.

[KieranRatcliffeInvertedAI]

Done.

[adscib]

Suggested change

	:func:`drive` calls to ensure the agents behave properly locally. The quadtree is constructed
	:func:`drive` calls to ensure the agents see all their neighbours. The quadtree is constructed

[KieranRatcliffeInvertedAI]

Done.

[adscib]

Consider pointing the reader to the documentation for drive instead. That has its downsides, but makes it less likely that this description gets out of sync from the one in drive.

[KieranRatcliffeInvertedAI]

I like this idea, I will do the same with large_initialize wherever possible because yeah it might be annoying but it is much more convenient for maintainability and less prone to mistakes and confusion.

[adscib]

Suggested change

	The number of agents allowed in a region before it must subdivide. Currently this
	value represents the capacity of a quadtree leaf node that will subdivide if the
	number of vehicles in the region passes this threshold.
	The number of agents allowed in a region before it must subdivide. Currently this
	value represents the capacity of a quadtree leaf node that will subdivide if the
	number of vehicles in the region passes this threshold. In any case,
	this will be limited to the maximum currently supported by :func:`drive`.

Assuming we clip this value at 100, which I think we should do.

[KieranRatcliffeInvertedAI]

Done.

[KieranRatcliffeInvertedAI]

Outstanding addition to include:

• Tests for large initialize
• Tests for large drive

[Ruishenl]

Before we merge, please make sure all the tests have passed. Right now we have been getting this error from the log:

tests/test_large_drive.py:38: in <module>
    initial_response = large_initialize(
../../../.cache/pypoetry/virtualenvs/invertedai-mfDNwQNM-py3.10/lib/python3.10/site-packages/pydantic/_internal/_validate_call.py:113: in __call__
    res = self.__pydantic_validator__.validate_python(pydantic_core.ArgsKwargs(args, kwargs))
invertedai/large/initialize.py:368: in large_initialize
    raise InvertedAIError(message=exception_string)
E   invertedai.error.InvertedAIError: Unable to initialize region at x=79.8 y=-4.3 with size 100.0 after 1 attempts. 

[KieranRatcliffeInvertedAI]

Before we merge, please make sure all the tests have passed. Right now we have been getting this error from the log:

tests/test_large_drive.py:38: in <module>
    initial_response = large_initialize(
../../../.cache/pypoetry/virtualenvs/invertedai-mfDNwQNM-py3.10/lib/python3.10/site-packages/pydantic/_internal/_validate_call.py:113: in __call__
    res = self.__pydantic_validator__.validate_python(pydantic_core.ArgsKwargs(args, kwargs))
invertedai/large/initialize.py:368: in large_initialize
    raise InvertedAIError(message=exception_string)
E   invertedai.error.InvertedAIError: Unable to initialize region at x=79.8 y=-4.3 with size 100.0 after 1 attempts. 

Absolutely. I ran those tests locally and they passed without issue so I think therein lies the cause for the failure. I will come back and solve this issue before merging.