global program options

[grahamlopez]

ASGarD asgardasgard Issues

16

Open Opened 5 months ago by Graham Lopez Close issue New issue global program options

to avoid polluting the APIs with program options, let's make a const singleton that can be globally accessed Related issues 0

Graham Lopez @bgl added someday label 5 months ago
Graham Lopez @bgl added hygeine label and removed feature label 4 months ago

[dlg0]

So following our discussion today, here I am looking into the testing of the element_table and I see this ...

// Construct forward and reverse element tables
element_table::element_table(options const program_opts, int const num_dims)
{
  int const num_levels     = program_opts.get_level();
  bool const use_full_grid = program_opts.using_full_grid();

which is exactly the type of thing I was doing in the matlab, but decided to avoid since it would complicate the testing. The way in which it would complicate the testing (at least what I thought would be a complication), is that you'd then have to construct the program_opts object to test something (same with the pde if you pass around the pde object a lot. But then I see this in the testing ...

  int const levels_2 = 3;
  int const dims_2   = 2;
  options o_2        = make_options({"-l", std::to_string(levels_2)});
  element_table t_2(o_2, dims_2);

which is exactly what is happening.

So, now I'm back to square 1 on how to approach this ... kind of. But I'm still going to start doing something.

[grahamlopez]

I'm not sure if I understand your question here, but we can talk in person if that's easier.

[bmcdanie]

unless you'd prefer not to @dlg, I think we should stick with the options object we have for now. it's usage is not pervasive enough to clutter APIs, and a regular object is simpler to use and understand than a singleton that needs runtime info for construction.

[dlg0]

I'm curious how a const singleton that is globally accessible is different from a simple object - presumably, both either can be constructed using default values or can take runtime info?

[bmcdanie]

@dlg0 this is true. it's certainly possible to do it either way.

suppose we want to go with a single object that is globally accessible through the program runtime.

can't make it static, needs runtime args, so we need checks to ensure nothing accesses it before init. essentially assert it's initialized before anyone queries it. then there is some little boilerplate type code to ensure it's a singleton. all of this adds a tiny amount of complexity but no big deal. it's also slightly more complex to set up tests for.

my main hold up - we also have defaults for level, degree, and dt loaded from the pdes, so options can't be const if it's going to extend beyond setting up the pde as it does now. so we have a global singleton that may be modified and read from anywhere. also, it's less clear where this may happen, since it's global and not explicitly passed anywhere. because most people are relatively sane, I don't think people will modify level degree and dt willy-nilly. but they could. so this adds a tiny bit of problem surface.

now suppose we want to go with a regular, scoped object. we can modify it to update with default PDE values in main, but everything that takes it as an argument takes it as const. we know those routines won't modify it, and we know whose reading from it and when. this costs a small amount of text in function signatures, so arguably less readable (clutter).

either way isn't show-stopping to me, I just think we have a slightly tidier code without the global object at the cost of a function argument in a few places. thanks for coming to my ted talk.

[bmcdanie]

third option is change the relationship between options, CLI parsing, and PDE. e.g. construct the PDE and the options object from the CLI parsing object. then you can have a global const singleton. in this case, I'm not opposed to going with a global, but I also don't think we gain much. also, we have to do all the sanity checking for command line options more or less twice. I'm sure there's other ways to restructure as well if we really want the global.

[dlg0]

Certainly, I'd think that it needs to be initialized when it is constructed and that not doing that has hurt us in the past with other things. I'm slowly moving in this direction in the matlab, and can do so more promptly. So really the main issue here is that we are changing the options after we construct the options object? If so, we should fix that.

[bmcdanie]

I think this is a good idea, it will just take refactoring a few small things around options. at that point, I'd still prefer a scoped object, but have no objections to a const global one.

[bmcdanie]

mostly my preference at that point would be based around the ease of explaining/reading the code - "we don't use globals, we always pass objects" vs "we have this one exception..."

[dlg0]

OK, so we can either close this and make a new issue - or perhaps rename this issue to make the options object be RAII os similar?

[bmcdanie]

sure, sounds good