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

Customize layer assignment problem #16

Merged
merged 19 commits into from
Feb 7, 2022
Merged

Customize layer assignment problem #16

merged 19 commits into from
Feb 7, 2022

Conversation

davide-f
Copy link
Contributor

@davide-f davide-f commented Apr 21, 2021

This commit enables to slightly change the layer assignment problem highlighted in #16 ; yet, for simplicity, nodes cannot be moved beyond layers containing their children nodes.
This change enables the creation of this map:
image
The original plot had nodes 6 at layer 3 and node 8 at layer 4, instead.

Proposal to account for changes in the labels ordering
src/layering.jl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2021

Codecov Report

Merging #16 (f3e03e9) into main (88117f5) will decrease coverage by 0.63%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #16      +/-   ##
==========================================
- Coverage   92.72%   92.09%   -0.64%     
==========================================
  Files           4        4              
  Lines         165      177      +12     
==========================================
+ Hits          153      163      +10     
- Misses         12       14       +2     
Impacted Files Coverage Δ
src/layering.jl 95.91% <87.50%> (-4.09%) ⬇️
src/zarate.jl 90.26% <100.00%> (+0.08%) ⬆️

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 88117f5...f3e03e9. Read the comment docs.

Minor changes to avoid unwanted warnings
Changing error(...) in @warn (error may be annoying)
Check vector length
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.

Sorry again for the delay in reviewing.
This seems really complex.
I think it might be simpler if rather than post-processing the layer_groups
this was implemented via modifying the dists before the layer_groups are constructed.

# basic form that just always does what the user asked for
force_layers!(dists, forced_layers::Dict{Int, Int})
    for (node_id, target_layer) in forced_layers
        dists[node_id] = target_layer
    end
    return dists
end

Which we can fancy up to add the other features.
For example:

force_layers!(graphdists, forced_layers::Dict{Int, Int})
    # must process from end to beginning as otherwise can't move things after to make space
    ordered_forced_layers = sort(collect(forced_layers), by=last; rev=true)
    for (node_id, target_layer) in ordered_forced_layers
        curr_layer = dists[node_id]
        if target_layer < curr_layer
            @warn "Ignored opt_layer_assign for node $node_id; curr layer ($curr_layer) > desired layer ($target_layer)"
        elseif any(dists[child] <= target_layer for child in outneighbors(graph, node_id))
            @warn "Ignored opt_layer_assign for node $node_id; as placing it at $target_layer would place it on same layer, or later than it's children."
        else
            dists[node_id] = target_layer
        end
    end
    return dists
end

src/layering.jl Outdated Show resolved Hide resolved
@davide-f
Copy link
Contributor Author

davide-f commented Feb 7, 2022

@oxinabox Sorry for my long delay; I added the test case for the additional feature.
Now it should work as suggested including also the testing.
I will merge these changes in the other pull request

@davide-f davide-f mentioned this pull request Feb 7, 2022
@davide-f
Copy link
Contributor Author

davide-f commented Feb 7, 2022

Not sure why codecov fails, however the CI works perfectly
@oxinabox May we fix this "soon" so to close the requests once for all (my bad for the delay...)

@davide-f
Copy link
Contributor Author

davide-f commented Feb 7, 2022

The coverage seems to decrease because there are @Warns that are not tested (clearly...), I think that the code should be fine

src/layering.jl Outdated Show resolved Hide resolved
src/zarate.jl Outdated Show resolved Hide resolved
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

I will merge after CI finishes

@oxinabox oxinabox merged commit baea9a4 into oxinabox:main Feb 7, 2022
@Warns
Copy link

Warns commented Feb 8, 2022

The coverage seems to decrease because there are @Warns that are not tested (clearly...), I think that the code should be fine

I think I was summoned here by mistake ^^

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.

4 participants