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

Application Object #33

Closed
henryiii opened this Issue Jan 30, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@henryiii
Copy link
Member

henryiii commented Jan 30, 2017

GooFit 2.0 could have an App object, which is created at the beginning of main and goes out of scope at the end. This object could do MPI construction/cleanup, command line parsing, and other tasks. The API changes in 2.0 are an ideal place to add this object.

This is the proposed interface:

GooFit::Application app("This is my program", argc, argv);

This is the only piece required to handle MPI setup and teardown. However, it is highly recommended to also "run" the application (below), since that handles standard options to all goofit programs, like printing info about the card/processor, and includes a help flag.

Adding options is simple:

int val=0;
app.add_option("-v,--val", val, "Please set this value", true);

This subclasses my CLI11 library and modifies it to support a ROOT like constructor. It can process ints, floats, and strings, as well as vectors of arguments, flags, and sets. It can also handle positional arguments, defaults, required options, validators, and sub-commands with callbacks.

To parse the arguments and exit if needed:

try {
    app.run();
} catch(GooFit::ParseError &e) {
    return app.exit(e);
}

All examples have been fully converted to use this new object; the pipipi0 fit was modified to take human readable input, please check with --help and look at the code. Please also look at product and convolution in the MPI branch, those have been changed to avoid memory leakage.

Current Application support:

  • Hidden -DGOOFIT_MPI CMake options finds MPI and activates setup/cleanup
  • Help, gpu device list, gpu card or OMP thread info
  • ROOT TApplication-like interface
  • Setting a device (careful: any threading code must also set device)
  • Timing info? Not currently included

Original request:
I’m wondering if you have considered adding a goofit_init/goofit_cleanup functions that would be required for initialization/cleanup? I would request this type of functionality to embed MPI_Init call/cleanup, which would be an easy way to hide the messy macro code from the user-side. No variables are cleaned up in the examples, are these meant to be cleaned up by user code? The cleanup would be useful to have the PDF’s free up their memory, I don’t see any memory cleanup occurring.

@henryiii henryiii added this to the V 2.0.0 milestone Jan 30, 2017

@hassec

This comment has been minimized.

Copy link
Member

hassec commented Jan 30, 2017

Well technically one should add a delete for every new that one uses in the examples...

So in principle I am all for the idea of having some cleanup method, but the nicest way would probably be to move to newer c++11 features like smart pointers, right?
But then again this is a major part of the current framework.. So probably quite a major rewrite of code.

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented Feb 16, 2017

Requested feedback:

I would like to have feedback on the namespaces:

The main application object is in the GooFit namespace.

GooFit::Application(...)

The various tools, like ParseError and the rest of the errors, the validators, and options are available from the CLI namespace, or from GooFit.

Options:

  • Leave the recommended access from the GooFit namespace. If we provide a GooFit modified version of something in CLI, this would make that easy. Currently the only modification is for App, but that is renamed Application à la ROOT so there is no worry there.
  • Don't put the items from CLI in GooFit, just using CLI for everything but GooFit::Application.
  • Put Application inside CLI (in goofit). I don't like this option much.
  • Put CLI in GooFit, GooFit::CLI::

What do you think?

@henryiii henryiii closed this Mar 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment