-
Notifications
You must be signed in to change notification settings - Fork 217
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
Make use of faster RNG implementation for ionization #1542
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. |
I added a WIP because it still depends on #1541. |
d925dfb
to
362f469
Compare
Waiting for #1547 |
#1547 has been merged |
@n01r please:
|
362f469
to
7edd7a3
Compare
* ::type (boost::mpl::bool_<>) | ||
*/ | ||
template<typename T_Object> | ||
struct UsesRNG |
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 trait looks fairly generic to me. Should it rather be in PMacc?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true as well for the HasFlag
trait, then? #1548
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that boost integral_constant
s 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 traits
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name must reflect that only species with ionizer can be handled
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.
@n01r rename: FilterIonsByRNG
add to description: @tparam T_MPLSeq sequence of particle species (ions) with ionization enabled
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]
7edd7a3
to
cc4047d
Compare
RNGFactory is only created when it is needed.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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