Skip to content

add node directly with default empty BnBNodeInfo#38

Open
Wikunia wants to merge 2 commits intomainfrom
feature-no-namedtupletools
Open

add node directly with default empty BnBNodeInfo#38
Wikunia wants to merge 2 commits intomainfrom
feature-no-namedtupletools

Conversation

@Wikunia
Copy link
Copy Markdown
Owner

@Wikunia Wikunia commented Jun 19, 2022

Implements #37

I'm not sure whether this approach is a reasonable one. There is a default BnBNodeInfo now which is used by a default constructor of an AbstractNode. The values of id, lb and ub are then updated when the node is actually added to the tree.

All docstrings need fixes but maybe you (@matbesancon ) can already say whether this solves the flexibility problem you had in mind.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 19, 2022

Codecov Report

Merging #38 (a849f84) into main (10f0b93) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #38   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          131       133    +2     
=========================================
+ Hits           131       133    +2     
Impacted Files Coverage Δ
src/Bonobo.jl 100.00% <100.00%> (ø)
src/node.jl 100.00% <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 10f0b93...a849f84. Read the comment docs.

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>
Comment on lines +48 to +51
Creates a node of type `Node` with id `node_id` and the named tuple `node`.
For information on that see [`set_root!`](@ref).
"""
function create_node(Node, node_id::Int, parent::Union{AbstractNode, Nothing}, node_info::NamedTuple)
function create_node(Node, node_id::Int, parent::Union{AbstractNode, Nothing}, node::AbstractNode)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is strange here, we have a function create node that already takes as input the created node?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Yes that's the reason why I don't like this approach same with the mutable but I wasn't able to come up with something better which is most likely the reason why I used the named tuple approach 🤣
I wanted to avoid a node and an inner node as the user should have access to both but I also don't want to pass both around all the time. I could make a parametric type which includes both info and then one can dispatch on the inner later but accessing the variables of the inner then is a a bit tedious. Or one would need to create a get property and set property function to make it simpler....
Did you have a completely different approach in mind?

node_nt = merge(bnb_nt, node_info)
return structfromnt(Node, node_nt)
bnb_node = BnBNodeInfo(node_id, lb, Inf)
node.std = bnb_node
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the default here assumes the node is mutable?

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.

2 participants