bugGNU Octave - Bugs: bug #61800, possible code simplification with...

 
 

bug #61800: possible code simplification with c++17 std::variant

Submitter:  A.R. Burgers <arb>
Submitted:  Wed 12 Jan 2022 09:38:22 AM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Other
Status:  None Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 29 Feb 2024 09:29:32 PM UTC, comment #8: 

Yes, for version 10 we'll be requiring C++17, so yes, we can consider using std::variant where appropriate.

I'm not sure about the changes outlined in the original submission.  The implementation of inv.cc and other files like that is as old as Octave and maybe we can just find an overall better way now?  The solution may end up using something like std::variant, but I don't we should just substitute std::variant in an attempt to tidy up a mess that we might possibly avoid completely.

John W. Eaton <jwe>
Group administrator
Thu 29 Feb 2024 08:58:34 PM UTC, comment #7: 

std::variant seems to be used by octave now

http://hg.savannah.gnu.org/hgweb/octave/rev/8a582d69173f

A.R. Burgers <arb>
Sun 13 Feb 2022 01:26:52 AM UTC, comment #6: 

We haven't decided whether we are ready to require C++17.

John W. Eaton <jwe>
Group administrator
Sat 12 Feb 2022 10:44:36 PM UTC, comment #5: 

Is Octave using the attached std::variant or was it rejected?

Anonymous
Thu 13 Jan 2022 11:30:37 AM UTC, comment #4: 
Arun Giridhar <arungiridhar>
Group Member
Thu 13 Jan 2022 05:21:55 AM UTC, comment #3: 

It would probably be best to open a new thread on the maintainers discourse forum to discuss requiring c++17 for future versions of Octave.

John W. Eaton <jwe>
Group administrator
Wed 12 Jan 2022 08:43:34 PM UTC, comment #2: 

C++17 is already 4.5 years old at this point and gcc has supported it since gcc 5, and it's default in gcc 11.

https://gcc.gnu.org/projects/cxx-status.html#cxx17

A search shows there are very few distros that ship compilers older than that:

https://distrowatch.com/search-mobile.php?pkg=gcc&relation=less&pkgver=5.0&distrorange=InLatest#pkgsearch

of which Scientific Linux is the largest and that was last updated in 2018, now discontinued. The results for Clang 5 are similarly old.

If there are benefits to Octave in using C++17 (such as the polymorphic allocator and the improvement in this report), it would be worth switching to it from Octave 8, which gives enough time for packages to catch up. No need to maintain two versions in parallel.

How do the maintainers feel about making current Octave 8 dev sources require C++17 once Octave 7 has been released?

Arun Giridhar <arungiridhar>
Group Member
Wed 12 Jan 2022 03:44:16 PM UTC, comment #1: 

jwe added another feature that requires C++17 that can be enabled with a configure switch in Octave 7.

Maintaining two versions of the same code for such a basic element of Octave as "octave_value" doesn't get me very excited, tbh.

Maybe, we could announce in the NEWS of Octave 7 that Octave will require C++17 in a future version (maybe Octave 9).
That could open the door for adopting that C++ version in core Octave while giving package developers some time to potentially adapt their code.

@jwe and others: Thoughts about this?

Markus Mützel <mmuetzel>
Group administrator
Wed 12 Jan 2022 09:38:22 AM UTC, original submission:  

An octave_value by its nature can represent multiple types. This can lead to convoluted code. To satisfy my own curiosity I had a look at std::variant to see how that can help.

For instance in inv.cc for diagonal matrices:


  if (arg.is_diag_matrix ())
    {
      rcond = 1.0;
      frcond = 1.0f;
      if (arg.iscomplex ())
        {
          if (isfloat)
            {
              result = arg.float_complex_diag_matrix_value ().inverse (info);
              if (info == -1)
                frcond = 0.0f;
              else if (nargout > 1)
                frcond = arg.float_complex_diag_matrix_value ().rcond ();
            }
          else
            {
              result = arg.complex_diag_matrix_value ().inverse (info);
              if (info == -1)
                rcond = 0.0;
              else if (nargout > 1)
                rcond = arg.complex_diag_matrix_value ().rcond ();
            }
        }
      else
        {
          if (isfloat)
            {
              result = arg.float_diag_matrix_value ().inverse (info);
              if (info == -1)
                frcond = 0.0f;
              else if (nargout > 1)
                frcond = arg.float_diag_matrix_value ().rcond ();
            }
          else
            {
              result = arg.diag_matrix_value ().inverse (info);
              if (info == -1)
                rcond = 0.0;
              else if (nargout > 1)
                rcond = arg.diag_matrix_value ().rcond ();
            }
        }
    }


Using std:variant this coding can easily be halved:


 if (arg.is_diag_matrix ())
    {
      rcond = 1.0;
      frcond = 1.0f;
      if (isfloat)
        {
          AnyDiag AD = arg.as_Any_fDiag ();
          result = std::visit([&info] (const auto &mat)->octave_value
                      {return mat.inverse (info);}, AD);
          if (info == -1)
            {
              frcond = 0.0f;
            }
          else if (nargout > 1)
            {
              frcond = std::visit([] (const auto &mat)->float
                      {return mat.rcond ();}, AD);
            }
        }
      else
        {
          AnyDiag AD = arg.as_Any_dDiag ();
          result = std::visit([&info] (const auto &mat)->octave_value
                      {return mat.inverse (info);}, AD);
          if (info == -1)
            {
              rcond = 0.0f;
            }
          else if (nargout > 1)
            {
              rcond = std::visit([] (const auto &mat)->double
                      {return mat.rcond ();}, AD);
            }
        }
    }


If the a condition number is not required, one could write pretty niftily:


      AnyDiag AD = arg.as_AnyDiag ();
      result = std::visit([&info] (const auto &mat)->octave_value
                      {return mat.inverse (info);}, AD);


The code to extract a diagonal matrix can be moved for reuse to the octave_value class:


AnyDiag octave_value::as_Any_dDiag (void) {
  if (iscomplex ())
    return complex_diag_matrix_value ();
  else
    return diag_matrix_value ();
}

AnyDiag octave_value::as_Any_fDiag (void) {
  if (iscomplex ())
    return float_complex_diag_matrix_value ();
  else
    return float_diag_matrix_value ();
}

AnyDiag octave_value::as_AnyDiag (void) {
  bool isfloat = is_single_type ();
  if (iscomplex ())
    {
      if (isfloat)
        return float_complex_diag_matrix_value ();
      else
        return complex_diag_matrix_value ();
    }
  else
    {
      if (isfloat)
        return float_diag_matrix_value ();
      else
        return diag_matrix_value ();
    }
}


The std::variant typedefs in liboctave/array/AnyDiag.h are these:


#if ! defined (octave_AnyDiag_h)
#define octave_AnyDiag_h 1

#include "CDiagMatrix.h"
#include "dDiagMatrix.h"
#include "fCDiagMatrix.h"
#include "fDiagMatrix.h"
#include <variant>

typedef std::variant <     DiagMatrix,      ComplexDiagMatrix,
                      FloatDiagMatrix, FloatComplexDiagMatrix> AnyDiag;
typedef std::variant <     DiagMatrix,      ComplexDiagMatrix> Any_dDiag;
typedef std::variant <FloatDiagMatrix, FloatComplexDiagMatrix> Any_fDiag;

#endif // octave_AnyDiag_h


So it appears there is scope for code reduction and simplification, but it requires c++-17, and I guess that makes it a bit of a long shot, as octave doesn't seem to use features beyond c++-11.

A.R. Burgers <arb>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #52649:  AnyDiag.cset added by arb (6KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by mmuetzel
  • -email is unavailable- added by arb (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-01-12 mmuetzel Carbon-Copy- Added jwe
    2022-01-12 arb Attached File- Added AnyDiag.cset, #52649

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code