search

gnu-octave / statistics

The Statistics package for GNU Octave
GNU General Public License v3.0
24 stars 22 forks source link

Package version 1.6.0 shadows several core library functions: is this intentional? #121

Closed sffc closed 7 months ago

sffc commented 7 months ago

Hi! I maintain Octave Online and the statistics package is one of the handful of packages that are auto-loaded by default into Octave Online sessions.

When upgrading from version 1.5.2 to 1.6.0 of the statistics package (and from Octave 7.4 to Octave 8.4), I noticed that there are function shadow warnings that were not present in the old version:

warning: function /usr/local/share/octave/packages/statistics-1.6.0/shadow9/var.m shadows a core library function
warning: function /usr/local/share/octave/packages/statistics-1.6.0/shadow9/mad.m shadows a core library function
warning: function /usr/local/share/octave/packages/statistics-1.6.0/shadow9/mean.m shadows a core library function
warning: function /usr/local/share/octave/packages/statistics-1.6.0/shadow9/std.m shadows a core library function
warning: function /usr/local/share/octave/packages/statistics-1.6.0/shadow9/median.m shadows a core library function

The docs for these functions in the statistics package versus standard Octave show that although the first argument is the same, the further arguments differ. This makes me worried that code that had been using the standard Octave versions of these functions might break upon landing this upgrade. The functions in question being very commonly used also increases the probability of Hyrum's Law.

I see a few paths forward:

  1. Remove the statistics package from the auto-load set in Octave Online. This may break code that depends on the statistics package being available, but it is an easy fix (add a pkg load statistics where needed) which the IDE can recommend. However, it may make the statistics package less discoverable.
  2. Revert to the latest version of the package that doesn't shadow core library functions.
  3. Suppress the shadow warnings and hope that userland code won't break.

I am leaning toward option 1 but wanted to post here to ask for feedback or ask if there is a roadmap for stopping the shadowing of these core library functions. Thank you!

pr0m1th3as commented 7 months ago

Hi, the five functions in the latest statistics version have been back ported to octave, but they 've been kept in the statistics package so that they are available in previous Octave versions as well. These functions, are fully MATLAB compatible (with some extra functionality regarding higher dimensions matrices) and don't break backward compatibility. Thus, there should be no problem with older code.

Their development and updated functionality was driven to address the needs of other newly implemented functions with version 1.6, which require the extended functionality which was not available in core Octave at the time. The shadowing will stop with Octave 9, which will include the latest dev versions for these functions. In MATLAB, these functions are part of the statistics and ML toolbox, but in Octave, they have been part of core Octave for a long time. As a result, it is not easy to move them from core Octave to the statistics package.

The reason for not suppressing the warnings was to inform the users for this situation. If you find this a bit intrusive for octave-online, then you can suppress them by adding the appropriate directive in PKG_ADD file of the statistics package. The 1.6.0 version has been a huge leap forward in terms for MATLAB compatibility as well as new functionality, so my suggestion is to include it octave-online, since this will allow a better integration of octave-online with statistics courses. The development in version 1.6 was mostly driven to address the needs for my statistics courses last year, hence the large update. There are things that break backwards compatibility in 1.6, but these relate to the functionality of the distribution functions, which have been updated to be fully MATLAB compatible. Older code using mean, std, var, mad, and median should work out of the box.

Hope this helps.

sffc commented 7 months ago

Thank you! Given your context around the backwards compatibility of these functions, I'll suppress the warning while running Octave 8.4.

https://github.com/octave-online/octave-online-server/commit/dff9d3950b65d1146dcc0fd6450779e6a5710ef7