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

Revise layer number in force_layer #20

Merged
merged 6 commits into from
May 17, 2022
Merged

Conversation

davide-f
Copy link
Contributor

@davide-f davide-f commented Feb 8, 2022

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
Copy link
Contributor Author

davide-f commented Feb 8, 2022

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

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2022

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
Copy link
Contributor Author

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

@oxinabox
Copy link
Owner

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:

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
Copy link
Contributor Author

@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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Owner

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
Copy link
Contributor Author

davide-f commented Apr 12, 2022

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

davide-f commented May 17, 2022

@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

Copy link
Owner

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@oxinabox oxinabox merged commit 5c743f9 into oxinabox:main May 17, 2022
@davide-f
Copy link
Contributor Author

No problem, thank you very much!

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

Successfully merging this pull request may close these issues.

3 participants