Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Another possible fix for #101.
Context
In principle
__add__
and__iadd__
should do the exact same thing, so in C++ one would overloadoperator+
and then overloadoperator+=
usingoperator+
. Things should be done similarly in practice. Up to now the two functions where using different implementations.Possible solutions
1: Fix the bug in the implementation of
__iadd__
Even though we can make the two different implementations do the exact same thing, I'd still argue that the strategy is un-intuitive and possibly bug-prone.
2: Implement
__add__
in the bindings and attach__iadd__
dynamically to the class in PythonThe strategy is:
__iadd__
fromtrees.h
.trees.py
file insrc/vampyr
containing:__init__.py
like so:I tried this, but it actually adds more code complexity Python-side! Part of the reason is that Python does not have a notion of templates, so attaching a method to
FunctionTree
has to be done once per value ofD
(3 times as things stand now) And actually instead of a singletrees.py
this needs to be done in 3 separate Python files plus the re-export in__init__.py
loses all notions of namespacing by dimension (vampyr1d
,vampyr2d
,vampyr3d
) I think this is hard to get right.3: Implement
__add__
as a free function instead of a lambda and re-use it for__iadd__
This is what I ended up doing in this PR.