Allow non-contiguous layers to be specified simultaneously in force_layer and force_equal_layers

[thchr]

Before this PR, it is possible to specify vertices in layer positions that skip certain layers. Then, internally, those skips are eventually removed.

However, this makes it tricky to combine with force_equal_layer because you cannot know ahead of time what the layer position will be. This PR goes to greater lengths to ensure that the requested layer position is respected, hopefully making it much easier to combine force_layer and force_equal_layers when force_layer is not a simple unit range.

@oxinabox

[oxinabox]

Ah so the idea is to keep the empty layers til later, so it is more predicatable as to what you are forcing where?

[oxinabox]

Note the different time complexity.
This is O(n*d) where as the previous code is O(n* log(n))

But it seems we can do better with

Suggested change

	layer_groups = [findall(==(d), dists) for d in 1:maximum(dists)]
	layer_groups = [Int[] for d in 1:maximum(dists)]
	for (ii, d) in enumerate()
	push!(layer_groups[d], ii)
	end

Which is only O(n + d)

[thchr]

Ah, I hadn't thought this. Your suggestion is very nice: I will add that now 👍 !

[thchr]

Would it be more natural to simply make this a Dict{Int, Vector{Int}} instead? Then there could also be negative indices and we wouldn't have to potentially store a bunch of empty layers.

Maybe there's some assumptions in the code elsewhere that assumes iteration over layer2nodes in incrementing fashion?

[oxinabox]

Would it be more natural to simply make this a Dict{Int, Vector{Int}} instead? Then there could also be negative indices and we wouldn't have to potentially store a bunch of empty layers.

My instinct is that in practical cases the extra costs from empty layers will be lower than the extra cost of needing to do a hash.

Where would we get negative layer indexes? Would that be just from users specifying them by hand?
So it would be a way to say to put this before the natural root layer?
That sounds like a bigger change that would want to be in its own PR

Maybe there's some assumptions in the code elsewhere that assumes iteration over layer2nodes in incrementing fashion?

Good question. I don't think so, but i can't from memory rule it out

[thchr]

Agree, better to leave that for a potential follow-up, if ever relevant. Thanks for expeditious merging 👍 !

[thchr]

Ah so the idea is to keep the empty layers til later, so it is more predicatable as to what you are forcing where?

Exactly 👍 . Also, for plotting purposes, one may know that it would be nice to have some separation between certain graph components - that can be achieved by "reserving" some unoccupied layers between connected components via force_layer now.