modifications to braid.hpp

[david8dixon]

I've made some changes to braid.hpp and I was wondering if you would accept a PR. Ultimately, my goal is to write this:

double time;
int iter, level, done;
astatus.GetTILD( &time, &iter, &level, &done );
int tindex;
astatus.GetTIndex(&tindex);

As this:

auto tild   = astatus.GetTILD();
auto tindex = astatus.GetTIndex();

Below is an example of what I've done. Note that I preserve the current API so this won't break downstream code:

struct TILD{
  double time = 0;
  int iters = 0;
  int level = 0;
  int done = 0;
};

// Wrapper for BRAID's AccessStatus object                                                                                                                                                                                                                    
class BraidAccessStatus
{
   private:
      braid_AccessStatus astatus;

   public:
      BraidAccessStatus(braid_AccessStatus _astatus)
      {
         astatus = _astatus;
      }

      auto GetTILD()
      {
        TILD tild;
        braid_AccessStatusGetTILD(astatus, &(tild.time), &(tild.iters), &(tild.level), &(tild.done));
        return tild;
      }
      auto GetT()                    { double result; braid_AccessStatusGetT(astatus, &result); return result;}
      auto GetTIndex()               { int result; braid_AccessStatusGetTIndex(astatus, &result);return result; }
      auto GetDone()                 { int result; braid_AccessStatusGetDone(astatus, &result); return result; }
      auto GetLevel()                { int result; braid_AccessStatusGetLevel(astatus, &result); return result; }
      auto GetNLevels()              { int result; braid_AccessStatusGetNLevels(astatus, &result); return result;}
      auto GetIter()                 { int result; braid_AccessStatusGetIter(astatus, &result); return result; }
      auto GetWrapperTest()          { int result; braid_AccessStatusGetWrapperTest(astatus, &result); return result; }
      auto GetResidual()             { double result; braid_AccessStatusGetResidual(astatus, &result); return result; }
      auto GetNRefine()              { int result; braid_AccessStatusGetNRefine(astatus, &result); return result; }
      auto GetNTPoints()             { int result; braid_AccessStatusGetNTPoints(astatus, &result); return result; }
      auto GetSingleErrorEstAccess() { double result; braid_AccessStatusGetSingleErrorEstAccess(astatus, &result); return result; }
      auto GetCallingFunction()
      {
         int result;
         braid_AccessStatusGetCallingFunction(astatus, &result);
         return result;
      }

      void GetTILD(braid_Real *t_ptr,
                   braid_Int  *iter_ptr,
                   braid_Int  *level_ptr,
                   braid_Int  *done_ptr)
      { braid_AccessStatusGetTILD(astatus, t_ptr, iter_ptr, level_ptr, done_ptr); }
      void GetT(braid_Real *t_ptr)                       { braid_AccessStatusGetT(astatus, t_ptr); }
      void GetTIndex(braid_Int *tindex_ptr)              { braid_AccessStatusGetTIndex(astatus, tindex_ptr); }
      void GetDone(braid_Int *done_ptr)                  { braid_AccessStatusGetDone(astatus, done_ptr); }
      void GetLevel(braid_Int *level_ptr)                { braid_AccessStatusGetLevel(astatus, level_ptr); }
      void GetNLevels(braid_Int *nlevels_ptr)            { braid_AccessStatusGetNLevels(astatus, nlevels_ptr);}
      void GetIter(braid_Int *iter_ptr)                  { braid_AccessStatusGetIter(astatus, iter_ptr); }
      void GetWrapperTest(braid_Int *wtest_ptr)          { braid_AccessStatusGetWrapperTest(astatus, wtest_ptr); }
      void GetResidual(braid_Real *rnorm_ptr)            { braid_AccessStatusGetResidual(astatus, rnorm_ptr); }
      void GetNRefine(braid_Int *nrefine_ptr)            { braid_AccessStatusGetNRefine(astatus, nrefine_ptr); }
      void GetNTPoints(braid_Int *ntpoints_ptr)          { braid_AccessStatusGetNTPoints(astatus, ntpoints_ptr); }
      void GetSingleErrorEstAccess(braid_Real *estimate_ptr)   { braid_AccessStatusGetSingleErrorEstAccess(astatus, estimate_ptr); }
      void GetCallingFunction(braid_Int *callingfcn_ptr)
      {
         braid_AccessStatusGetCallingFunction(astatus, callingfcn_ptr);
      }
                                                                                                                                                                                 
      ~BraidAccessStatus() { }
};

[jbschroder]

Hi David, Thanks for the interest in XBraid!  I hope that you're enjoying it.  

I can definitely see how the suggested changes lead to more compact user code.  On the developer side, though, this would leaves us with two interfaces to maintain, the "auto" interface and the "void" interface.  The user code "auto tild = astatus.GetTILD();" is also a bit opaque, but compact. I like it, but also don't find it as easy to understand. I suppose you might say that I'm on the fence.

@rfalgout Let us know if you have any thoughts.

Cheers,

Jacob

[david8dixon]

I agree that it is opaque but I wanted to preserve the naming convention that was already there. While the call site might be more opaque, we could choose a different name. Further, it prevents people from doing the following:

double time;
int iter, level, done;
astatus.GetTILD(&time, &level, &done, &iter); // note that the arguments were passed incorrectly

The above code will compile and while it is perhaps clear that you are setting variables pointed to by the pointers passed in, it is erroneous.

As an alternative, we could return a tuple rather than a struct and if you can support c++17 structured bindings one can:

auto [time, tindex, level, done]   = astatus.GetTILD();

However, one could also accidentally reorder the arguments and wind up with a bug.

None of this changes the fact that you would have to maintain two APIs (unless of course you eventually deprecate one). Do you find yourself making changes to this header file often? If not, I would happily volunteer to help as we depend on Xbraid and I would rather not maintain a fork.

[rfalgout]

Hi @david8dixon and @jbschroder . After staring at this for a while, I guess I don't see a strong enough advantage to justify adding it to braid. Also, I think the status feature in braid could still be further improved (we've already overhauled it once before), so I expect it may change. My 2 cents.

Thanks for making the suggestion, @david8dixon ! I really appreciate that you took the time to write a clear detailed Issue, too!

[jbschroder]

I'm closing this for now...but we can re-open it if needed.