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
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( Jump to the original submission )

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>
Project 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>
Project 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>
Project 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>
Project 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>
Project Member
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 project 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.9