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

[cholm]

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,

-- 
Christian Holm Christensen 
-------------------------------------------------
  Niels Bohr Institute, Blegdamsvej 17, DK-2100 Copenhagen
  http://cern.ch/cholm, +4524618591