[Rivet] Patch for Rivet to automatically detect available FastJet plugins

[Andy Buckley]

Hi Christian,

I'm afraid we disagree on a lot of aspects of how Rivet should work, 
coding standards, etc. ;-)  My preference is still that Rivet should 
insist on having the same functionality in its external libraries on 
every install, and we should definitely not have dependency-driven 
variations in which analyses are built and installed.

We will anyway shortly have to deal with the more awkward problem of 
fjcontrib, where things really are out of control. I'll bear your 
solution in mind for that and hopefully we can somewhere that pleases 
~everybody.

Andy

PS. I'm still interested to see a standalone rivet-nopy patch. Cheers!


On 09/05/17 13:19, cholm wrote:
> Hi Andy,
>
> On 2017-05-08 12:41, Andy Buckley wrote:
>> Hi Christian,
>>
>> As I mentioned separately, I'm not sure about this patch. I think
>> Rivet should have consistent runtime behaviour, and throwing runtime
>> exceptions for problems in the configuration of external libraries
>> seems less desirable than just to throw an error at configure time and
>> request that fastjet be properly configured.
>
> What "properly configured" means depends.  Some installations of FastJet
> may not have all plugins enabled for various reasons - conflicts,
> platforms, licensing.  For example, the standard Ubuntu/Debian FastJet
> packages do not contain most of the CDF plugins (as far as I remember)
> because of licensing issues.  In other words, there's no one "standard"
> FastJet installation.  A package, such as yours, must be able to cope
> with those kinds of situations.
>
> Now, the exception thrown by the patch is done at init-time - that is,
> right up front.  In that way, the user will know immediately that
> something is wrong/missing/mis-configured/.... Furthermore, warnings are
> issued at configure-time that some plugins are not available.
>
> Another option is to simply not build the analyses that depend on
> missing plugins.  That does, however, require a bit more of the build
> system, but is certainly doable.  Essentially you would exploit Automake
> conditionals.
>
>> Also, from the patch I see that your conversions to use your
>> makePlugin() are actually less compact and more "C++-noisy" than what
>> was there before
>
> You are interfacing a "legacy" library that expects things in a
> particular way - if you want to do that robustly, then one needs a bit
> of "ugly" coding.
>
>> -- at the same time as restricting the fj plugin
>> constructor signature to just two doubles.
>
> That's easily remedied by using std::initializer_list - e.g.,
>
>   struct FastJet {
>     // This is never implemented to throw a compile-time exception if a
> given,
>     // unknown, plugin is requested.
>     template <typename PLUGIN>
>     PLUGIN* makePlugin(std::initializer_list<double> l);
>     ...
>   };
>   #ifdef HAVE_FASTJET_SISCONEPLUGIN_HH
>   template <>
>   fastjet::SISConePlugin*
> FastJet::makePlugin(std::initializer_list<double> l)
>   {
>     if      (l.size() < 1) return new fastjet::SISConePlugin();
>     else if (l.size() < 2) return new fastjet::SISConePlugin(*l, 0.75);
>     return fastjet::SISConePlugin(*l++, *l);
>   }
>   #endif
>   // and more specialisations
>
>   // call
>   fastjet::SISConePlugin* = FastJet::makePlugin<SISConePlugin>({0.5,
> 0.75});
>
>> This is actually going in
>> the opposite direction from what we've been recently doing, which is
>> to allow more direct use of fj constructors, etc.
>
> That, I would recommend against.  Since you're wrapping the FastJet code
> in a Projection, it seems that one must encourage - if not enforce -
> using that interface.
>
>> That, and the very
>> "bitty" #ifdef-y implementation of makePlugin in order to add a
>> runtime exception behaviour is not something I'm comfortable with.
>
> See the above reasons why a run-time exception isn't all that bad, or
> for an alternative that exploits template specialisations, causing a
> compile-time error.
>
>> I'm maybe missing the motivation/necessity of this behaviour, though.
>> What exactly is the use-case that you wrote this patch to handle? Is
>> there a reason why it wouldn't be acceptable to just test for all the
>> fj plugin headers given in your patch, and throw a configure ERROR if
>> any are missing?
>
> Of course you can make configure fail - it will just limit the usability
> of your package to the explicit cases where people have built their own
> FastJet library - which is pretty silly given that it's readily
> installable on most GNU/Linux distros.
>
> My recommendation:
>
> - Change the `makePlugin' to use template specialisations.
> - Only build Analyses that used the plugins supported by the target system
> - Be careful to define your preprocessor flags in some development
> header file (i.e., a "public" header file that 3rd party code can include).
>
> The reason I didn't do the template specialisations was a) I didn't
> think of it b) the other way encodes the selection into compiled code so
> that it is fixed at the time of building the Rivet library.
>
> BTW, I think it's a bad idea that you import the full std:: namespace
> into the Rivet namespace.  If you really want, you could selectively
> import classes/structs/...
>
>   namespace Rivet
>   {
>      using std::vector;
>      ...
>   }
>
> Yours,
>


-- 
Dr Andy Buckley, Lecturer / Royal Society University Research Fellow
Particle Physics Expt Group, University of Glasgow