-
-
Notifications
You must be signed in to change notification settings - Fork 17
added NPRK trees functionality in src/NPRKTrees.jl and tests in test/… #187
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
base: main
Are you sure you want to change the base?
Conversation
…nprktrees_test.jl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your contribution. Please find below some general comments that also apply to other parts of the code. Please let me know if I can help you with that.
# argument root_color_fixed = true for NPRK trees, i.e., checks if edges of the NPRK tree are multicolored | ||
# argument root_color_fixed = false for ARK trees, i.e., checks if nodes of the NPRK tree are multicolored | ||
# corresponds to additive coupling conditions | ||
function isMultiColored(colored_tree::ColoredRootedTree, root_color_fixed::Bool)::Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function isMultiColored(colored_tree::ColoredRootedTree, root_color_fixed::Bool)::Bool | |
function is_multi_colored(colored_tree::ColoredRootedTree, root_color_fixed::Bool) |
In Julia, the general convention is to use snake_case
for function and variable names, see https://docs.julialang.org/en/v1/manual/variables/#Stylistic-Conventions. Moreover, it's not typical to assert the return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Hendrik for your comments; I am not too familiar with Julia conventions so I appreciate the suggestions. I will revise when I get the chance.
return ~allequal(colorseq) | ||
else | ||
return ~allequal(colored_tree.color_sequence) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return ~allequal(colorseq) | |
else | |
return ~allequal(colored_tree.color_sequence) | |
return !allequal(colorseq) | |
else | |
return !allequal(colored_tree.color_sequence) |
~
is bitwise not, while !
is the general not operation that is more common in Julia code.
function GetParentIndex(levelseq::Vector, nodeindex::Int64)::Int64 | ||
toReturn = 0 | ||
for i in reverse(1:length(levelseq)) | ||
if levelseq[i] == levelseq[nodeindex]-1 | ||
toReturn = i | ||
break | ||
end | ||
end | ||
return toReturn | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function GetParentIndex(levelseq::Vector, nodeindex::Int64)::Int64 | |
toReturn = 0 | |
for i in reverse(1:length(levelseq)) | |
if levelseq[i] == levelseq[nodeindex]-1 | |
toReturn = i | |
break | |
end | |
end | |
return toReturn | |
end | |
function get_parent_index(levelseq::AbstractVector, nodeindex::Integer) | |
to_return = 0 | |
for i in reverse(eachindex(levelseq)) | |
if levelseq[i] == levelseq[nodeindex] - 1 | |
to_return = i | |
break | |
end | |
end | |
return to_return | |
end |
See the comments on the style above. In general, Julia does not require strict type constraints in function arguments; see, e.g., https://docs.julialang.org/en/v1/manual/style-guide/#Avoid-writing-overly-specific-types
# Computes the elementary weight of an NPRK tree for a given | ||
# NPRK tableau specified by: A cubic 3-tensor and b square matrix | ||
# corresponds to a 2-partition F(y,y) | ||
function elementaryWeightNPRK2(Atens::Array, bmatrix::Array, NPRK_tree::ColoredRootedTree)::Float64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is again too restrictive - we would like to be able to compute with Float32
, rational numbers, symbolic expressions etc. as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add using Test
and add some @test
statements explicitly to this file so that test results do not need to be checked manually but are checked automatically?
adding functionality for NPRK trees in NPRKTrees module and tests in test/nprktrees_test.jl including:
src/NPRKTrees.jl
ARKTreeIterator
: returns aVector
ofColoredRootedTree
s containing all (non-equivalent) ARK trees of a given number of colors and given orderNPRKTreeIterator
: returns aVector
ofColoredRootedTree
s containing all (non-equivalent) NPRK trees of a given number of colors and given orderisMultiColored
: determines whether aColoredRootedTree
(either ARK or NPRK tree specified as aBool
in the argument) contains multiple colorsGetParentIndex
: given a node in a tree, returns the index in the level sequence of the parent nodeisColorBranching
: determines whether an NPRK tree is color branching, i.e., contains at least one node with two outward edges of differing colorselementaryWeightNPRK2
: given an NPRK tableauresidualNPRK2
: computes the residual of the order condition with the same as input as above, i.e.,test/nprktrees_test.jl