nutofem / nuto

NuTo - yet another finite element library
https://nuto.readthedocs.io
Boost Software License 1.0
17 stars 5 forks source link

Add functionality to groups #268

Closed vhirtham closed 6 years ago

vhirtham commented 6 years ago

I added a variadic template function that unites more than two groups. So instead of writing something like:

auto unitedGroup = Unite(group1, Unite(Unite(group2, group3), Unite(group4, group5));

you can now write:

auto unitedGroup = Unite(group1, group2, group3, group4, group5);

This works for an arbitrary number of groups. Test is also included.

codecov[bot] commented 6 years ago

Codecov Report

Merging #268 into develop will increase coverage by 0.01%. The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #268      +/-   ##
===========================================
+ Coverage    90.86%   90.88%   +0.01%     
===========================================
  Files          326      326              
  Lines        10906    10924      +18     
===========================================
+ Hits          9910     9928      +18     
  Misses         996      996
Flag Coverage Δ
#integrationtests 66.45% <ø> (ø) :arrow_up:
#unittests 96.25% <100%> (ø) :arrow_up:
Impacted Files Coverage Δ
test/base/Group.cpp 100% <100%> (ø) :arrow_up:
nuto/base/Group.h 100% <100%> (ø) :arrow_up:

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 8708207...d2a7e05. Read the comment docs.

vhirtham commented 6 years ago

I changed some code addressing the remarks of @Psirus . I also added the direct access operator and a corresponding test.

Psirus commented 6 years ago

Why do we need the direct access operator?

vhirtham commented 6 years ago

Can't come up with an example right now, but there were some occasions in the past, where I wanted to use it. @pmueller2 also mentioned it to be a nice addition. I think it doesn't hurt, since it just a pass through function using the underlying base types functionality. Additionally I prefer something like this

std::vector<Foo*> expected = {&a, &b, &c, &e, &d};
for (unsigned int i = 0; i < unionGroup.Size(); ++i)
       BOOST_CHECK(unionGroup[i] == *expected[i]);

instead of

int i = 0;
std::vector<Foo*> expected = {&c,&e,&d,&a,&b};
for (auto& entry : unionGroup)
    BOOST_CHECK(entry == *expected[i++]);

I think the first variant of comparing entries of two containers is a little bit clearer than using an indexed access and an iterator access. YMMV.

To sum it up: We might not need it, but it might be handy in some cases and it does not hurt.

YMMV.

TTitscher commented 6 years ago

Two sorries, 1st for being late, 2nd for putting it here instead of an issue, I'll make a separate issue soon.

IMO the direct access operator goes beyond the original design idea of the group - a container holding references to non-duplicate objects in some deterministic way. With this access we suddenly have questions like: What is the fifth element after uniting [1,2,3] and [4,5]? Or better: How should unite be implemented so that it is intuitive for the user?

I would solve this differently, by allowing an iterator-like interface to all the functions that currently use a Group. This could be done with a std::span or similar implementations in lower c++ standards, e.g. this one. Then, no one is forced to use the groups. If you need vector-like access - use a vector. If you need a lot of set-like functions and uniqueness - use a set.