Revise layer number in force_layer

[davide-f]

In the previous pull request, to force node 5 in layer 3, it was needed to set [5=>2], which is a bit ackward as the entire Julia is 1-based.
In this request we fix that, so to specify node 5 in layer 3, the code [5=>3] shall be provided

[davide-f]

@oxinabox This is a simple fix, if you prefer the 0-based layer, simply close this PR.
Thank you!

[codecov-commenter]

Codecov Report

Merging #20 (87421ae) into main (6580041) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 87421ae differs from pull request most recent head 63ca2b0. Consider uploading reports for the commit 63ca2b0 to get more accurate results

@@           Coverage Diff           @@
##             main      #20   +/-   ##
=======================================
  Coverage   92.39%   92.39%           
=======================================
  Files           4        4           
  Lines         184      184           
=======================================
  Hits          170      170           
  Misses         14       14           

Impacted Files	Coverage Δ
src/layering.jl	95.91% <100.00%> (ø)

---

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 6580041...63ca2b0. Read the comment docs.

[davide-f]

@oxinabox Any comment on this pull request? Sorry for bothering

[oxinabox]

Oh sorry.
I thought I commented but something must have gone wrong as my comment doesn't show up.

What do you think if we replaced:

LayeredLayouts.jl/src/layering.jl

Line 4 in c992a7e

dists = longest_paths(graph, sources(graph))

with

layer_inds = longest_paths(graph, sources(graph)) .+ 1

and then make corresponding renames through out.
Which should remove the need to mess around with -1 all over the place.

Since a dist[x]==0 from the root, means x has layer_inds[x]==1

We might also need a -1 at the end before plotting to shift everything back one step to the left, I am not sure

[davide-f]

@oxinabox no problem. The fact is that -1 applies only to the layers that are forced by the user and there is not an easy way to do .-1 only to the second term of a pair, without heavy notation that I think is not needed.

The possible solutions I see are:

1. In the forloop to overwrite the value of the layer beforehand its -1 use.
2. The current approach

What do you think?

[oxinabox]

But we wouldn't need to subtract 1 if we used layer_ind rather than dist to determine where things were currently positioned?
Sorry, I am missing something

[davide-f]

The problem I stated arises only for the force_layers specified by the user.
Let's say the user wants to put node 5 into layer 3, the force_layer the user shall specify is [5=>3].
However, as you stated, when we need to force the layers, we shall modify dists that is 0-based: layer 3 corresponds to dist value 2.
That's why there is that -1 only in the force_layer.
When the user does not specify anything, dists is fine as it is now and in fact force layer would not make any change

[oxinabox]

When the user does not specify anything, dists is fine as it is now and in fact force layer would not make any change

But dists would also be fine if it was 1 larger and called layer_inds
It's basically only it's relative values that matter for the layout algorithm

[davide-f]

When the user does not specify anything, dists is fine as it is now and in fact force layer would not make any change

But dists would also be fine if it was 1 larger and called layer_inds It's basically only it's relative values that matter for the layout algorithm

Absolutely agree; the algorithm works in relative terms: we could change dists to be 1-base (currently is 0-based) or keep dists to be 0-based and adapt the layer_inds to match the expected behaviour by the user to specify the layer in terms of 1,2,... also according to Julia convention (vectors are 1-based) and the examples and tests used in which the layers are numbered from 1 to N.

Since dists is only an internal variable, changing all the code to adapt an internal variable I think is not worthy.
That may create other bugs in other areas of the code, more "-1/+1" for adapting the conventions and may take longer to code. That's why I was proposing this PR and simply adapt the layer_inds to be 1-based while keeping dists to be 0-based

[davide-f]

@oxinabox Any updates on this PR? It would be nice to finalize it so that this PR (daschw/SankeyPlots.jl#22) can be finalized as well.
Thank you

[oxinabox]

Sorry I have been away for work and then I got covid.
The problem i have with this is code like target_layer-1 < curr_layer where is non-obvious why it is being done.

And the answer is because target_layer is 1-based and cur_layer is zero based.
So we should either make cur_layer be 1 based also, or rename it to curr_layer_dist or something.

[davide-f]

@oxinabox I think I found a good way to make dists 1-based. Luckily the absolute value of dists is not used in the algorithm, hence the change was easier than expected.
May you crosscheck the current version?
Personally, I cannot spend too much more time on this

[davide-f]

@oxinabox Sorry for the pressing, but could be possible to finalize this and the parallel PR soon? I'd like to close these current open chapters and the several PRs pending this in other packages, possibly.
Thank you and many sorry

[davide-f]

@oxinabox Please, could you please close also this PR? it is a simple +1 to change the ordering...
I'd really love to finalize this.

Moreover, after that, if you could launch the registrator would be perfect

[oxinabox]

LGTM.
Thanks.
This seems the ideal solution.

Sorry about the extreme time it has taken between reviews.
I get like a dozen notifications i have to attend to each day, for better or worse mostly higher priority than this.
and I have a day job.

[davide-f]

No problem, thank you very much!