Make use of faster RNG implementation for ionization #1542
Conversation
|
Very (!) naive runtime testing of the branch: Ions activated, Ionization with before after speedup: 6.43x (ratio of the calculation simulation times) |
|
Nice! Is this the speedup you expected? |
|
Yes, very much so! It will change with different domain sizes and with the number of GPUs used, of course but I expected a speedup of about 5-10x. It's finally feasible to run simulations with ADK and whatever else I'll implement from now on. |
PrometheusPi
added
the
refactoring
|
I added a WIP because it still depends on #1541. |
n01r
force-pushed
the
topic-useFasterRNG
branch
from
d925dfb
to
362f469
Compare
|
Waiting for #1547 |
|
#1547 has been merged |
|
@n01r please:
|
n01r
force-pushed
the
topic-useFasterRNG
branch
from
362f469
to
7edd7a3
Compare
| * ::type (boost::mpl::bool_<>) | ||
| */ | ||
| template<typename T_Object> | ||
| struct UsesRNG |
There was a problem hiding this comment.
This trait looks fairly generic to me. Should it rather be in PMacc?
There was a problem hiding this comment.
Note: It is more common to have a value result defined as a member value rather than a subtype. This allows e.g. deriving from boost::true_type which is much more compact.
In general: Type results as type value results as value and a bool certainly is a value
There was a problem hiding this comment.
But isn't boost::mpl::bool_<C> a type? Just so that boost::mpl::bool_<C>::value == C.
There was a problem hiding this comment.
This is correct. But with this trait you want a value as its return value, not a type. Or is there any benefit in returning type<true> instead of true?
Examples: boost::make_unsigned<...> obviously returns a type, but boost::is_unsigned<...> also obviously returns a bool value(!).
There was a problem hiding this comment.
Is this true as well for the HasFlag trait, then? #1548
There was a problem hiding this comment.
I stand corrected: I forgot the ...::type in the above example which turned it into unevaluated expressions not triggering the error.
As per offline discussion with @ax3l the solution is to inherit from bmpl::bool_<true/false> like in the following, corrected example:
#include <boost/mpl/vector.hpp>
#include <boost/mpl/copy_if.hpp>
#include <boost/mpl/if.hpp>
#include <boost/type_traits.hpp>
namespace bmpl = boost::mpl;
template<class T>
struct Foo: public bmpl::bool_<true>{};
template<>
struct Foo<int>: public bmpl::bool_<false>{};
typedef bmpl::vector<int, unsigned> MyVec;
typedef typename bmpl::copy_if<MyVec, boost::is_unsigned<bmpl::_1> >::type Result;
typedef typename bmpl::copy_if<MyVec, Foo<bmpl::_1> >::type Result2;
typedef typename bmpl::copy_if<MyVec, bmpl::and_<Foo<bmpl::_1>, boost::is_unsigned<bmpl::_1> > >::type Result3;
typedef typename bmpl::if_<bmpl::and_<Foo<int>, Foo<unsigned> >, int, bool>::type Result4;
int main(){
if(Foo<int>::value) return 1;
else return 0;
}
There was a problem hiding this comment.
yep that is wonderful, removes the useRNG run-time only function need and makes the world a pretty place.
There was a problem hiding this comment.
Note that boost integral_constants are in fact nullary metafunctions. See their definition:
[...]struct integral_constant : public mpl::integral_c<T, val>[...]
This is why the above use of boost::is_unsigned works. This is defined here and also in C++11
So as a matter of fact you can (and should) inherit from boost::true_type/boost::false_type which was my original statement :)
For more complex checks one can use the same approach as integral constants:
template<class T> class MyTrait{
typedef MyTrait<T> type; // For metafunction capability
[...] // Some checks and finally:
BOOST_STATIC_CONSTEXPR bool value = <value-based-on-checks>;
}
Note that deriving from true_type/false_type also allows if(is_unsigned<int>()) due to conversion operators defined, which is not done in the more complex case.
There was a problem hiding this comment.
also, correcting myself, they are mpl compatible: boost note.
nevertheless, @Flamefire your code snipped is outdated, recent versions of boost type traits (see link) are not inheriting from mpl::integral_c any more: (code, compatibility note) - but still are compatible nullary meta-functions.
@n01r use:
#include <boost/type_traits/integral_constant.hpp>
// default:
template<typename T_Object>
struct UsesRNG : public boost::false_type
{
};and specialize
#include <boost/type_traits/integral_constant.hpp>
namespace traits
{
template<typename T_IonizationAlgorithm, typename T_DestSpecies, typename T_SrcSpecies>
struct UsesRNG<particles::ionization::ADK_Impl<T_IonizationAlgorithm, T_DestSpecies, T_SrcSpecies> > :
public boost::true_type
{
};
} // namespace traitsThere was a problem hiding this comment.
@n01r pls implement to finalise PR
|
@PrometheusPi there you go 😉 |
|
@n01r thank you |
| * \TODO rather pass a sequence of objects, like ionization modules | ||
| */ | ||
| template<typename T_MPLSeq> | ||
| struct FilterByRNG |
There was a problem hiding this comment.
The name must reflect that only species with ionizer can be handled
There was a problem hiding this comment.
@n01r rename: FilterIonsByRNG
add to description: @tparam T_MPLSeq sequence of particle species (ions) with ionization enabled
There was a problem hiding this comment.
actually dependency logic is messed up in this place already.
RNG might be required by a species independent of ionization, too.
There was a problem hiding this comment.
offline result: @n01r will fix that in a follow-up PR (not important for release)
|
transferred review powers by @PrometheusPi - thank you for the great work guys! |
Exchange the RNG (Random Number Generator) for a faster version in ADK_Impl.hpp (and all future ionization models). Instead of creating new states for the RNG during each time step the state is now saved. It has the dimensions of the grid and is mapped to the cells. Whenever a new random number is created the state changes in that cell. Therefore it is also unique between time steps and particle species but the RNG does not do any superfluous work. Kudos to @Flamefire Status: compiles [x], runs [x]
`ionizationMethods.hpp`:
removed the old implementation that created unique seeds and RNG states
per time step, species, cell and MPI rank
Status: compiles [x], runs [x]
n01r
force-pushed
the
topic-useFasterRNG
branch
from
7edd7a3
to
cc4047d
Compare
RNGFactory is only created when it is needed.
n01r
force-pushed
the
topic-useFasterRNG
branch
from
cc4047d
to
44ecfed
Compare
| /* \todo fix: cannot PMACC_ALIGN() because it seems to be too large */ | ||
| /* random number generator */ | ||
| typedef PMacc::random::RNGProvider<simDim, PMacc::random::methods::XorMin> RNGFactory; | ||
| typedef PMacc::random::distributions::Uniform<float> Distribution; |
There was a problem hiding this comment.
This must be a float_X!
Important refactoring
Greatly overdue, the random number generator for the ADK model (and all future rate models for ionization) has been exchanged.
Before we used an RNG that was creating new states during each time step of the simulation which were unique for each step, particle species, cell and MPI rank.
Now the state is created only once and then saved as well as changed for each cell after each use. This implies the uniqueness of the seed for the aforementioned conditions and reduces workload at the same time.
This now allows for faster testing of all the ionization models and more importantly: highly increased simulation speed for more sophisticated ionization models than BSI.
Big kudos to @Flamefire ! 🎉
This pull request depends on the changes in #1541