Addition of stdlib_experimental_stats function mean

[jvdp1]

Here is a proposal of a function mean in stdlib_experimental_stats (see #113 for discussions and comments on the API). The API of mean follows the one of Fortran functions such as sum, minval, maxval (except for mask that is not implemented (yet) in mean).

The function mean supports arrays with up to rank 15 for Fortran2003 and higher; otherwise, it supports up to rank 7 (for CI on Ubuntu 7).

All files were generated with fypp and the fypp files are still in the directory src (fypp comments could be probably cleaner). The .f90 files are probably easier to use for checking and commenting.

Note: I modified the Makefile and CMake files but it need to be checked carefully.

A Markdown spec file is in src.

To do in a following PR (if accepted): if this PR is accepted and merged: addition of mask as optional argument for mean, to fully follow Fortran conventions. It should be easy to implement when the current proposition is validated.

[certik]

Overall it looks good to me as a start.

Couple questions:

1. Couldn't the f03_stdlib_experimental_stats and f90__stdlib_experimental_stats be combined? Which compiler does not support the modern syntax?

2. Inside f03_stdlib_experimental_stats, couldn't the contents be generated using fypp also? It seems repetitive.

[jvdp1]

1. Couldn't the f03_stdlib_experimental_stats and f90__stdlib_experimental_stats be combined? Which compiler does not support the modern syntax?

The compiler in the CI Ubuntu 7 does not support arrays with rank >7.

2. Inside f03_stdlib_experimental_stats, couldn't the contents be generated using fypp also? It seems repetitive.

@certik No sure to understand the question. Both files f90_stdlib_experimental_stats.f90 and f03_stdlib_experimental_stats.f90 are generated from stdlib_experimental_stats.fypp with fypp. Same thing for fxx_stdlib_experimental_stats_mean.f90. The flag VERSION90 is used to generate f90_ or f03_ files.

[certik]

Ah ok, so all the repetitive files are generated from a simple template. That answers my question.

What is Ubuntu 7? Do you mean GFortran 7?

I am fine with the PR as is for now. Later on I think we should not check in generated files, as discussed in #35.

[jvdp1]

Ah ok, so all the repetitive files are generated from a simple template. That answers my question.

Yes, indeed.

What is Ubuntu 7? Do you mean GFortran 7?

GFortran from the Github actions under the name 'ubuntu-latest, 7) (see here for some details, before I modified the CMake files)

Here is the reported message:

/home/runner/work/stdlib/stdlib/src/stdlib_experimental_stat.f90:134:44:

 real(sp), intent(in) :: x(:,:,:,:,:,:,:,:)
                                        1

Error: Array specification at (1) has more than 7 dimensions
/home/runner/work/stdlib/stdlib/src/stdlib_experimental_stat.f90:138:44:

 real(sp), intent(in) :: x(:,:,:,:,:,:,:,:,:)
                                        1

Error: Array specification at (1) has more than 7 dimensions
/home/runner/work/stdlib/stdlib/src/stdlib_experimental_stat.f90:142:44:

 real(sp), intent(in) :: x(:,:,:,:,:,:,:,:,:,:)
                                        1

Error: Array specification at (1) has more than 7 dimensions

I am fine with the PR as is for now. Later on I think we should not check in generated files, as discussed in #35.

I agree we should not check in generated files. However, no decision has been made in #35. So I prefered to keep both fypp files and the generated files. When a decision will be made in #35, generated files will be removed.
I guess that later CMake should trigger the generation of the f90 files (but I don't know how to do that).

[milancurcic]

Mostly looks good. The spec doc needs work, see my comments and question there.

Sorry to be so pedantic about this. Considering that this PR comes in before we have a spec template, we need to get the formatting right, otherwise it will only call for another PR. We can nail these things down here.

[jvdp1]

Sorry to be so pedantic about this. Considering that this PR comes in before we have a spec template, we need to get the formatting right, otherwise it will only call for another PR. We can nail these things down here.

@milancurcic No problem. I will integrate them when the question about dim will be solved.

[ivan-pi]

How complicated would it be to use more standard variable names such as i,j,k,l,m,n...? Maybe it is worth discussing whether we care about the style of auto-generated code.

I cannot find the place where you encoded the 132 line character limit. The generated code seems to obey this rule:

do i______________ = 1, size(x, 15)
     do i_____________ = 1, size(x, 14)
      do i____________ = 1, size(x, 13)
       do i___________ = 1, size(x, 12)
        do i__________ = 1, size(x, 11)
         do i_________ = 1, size(x, 10)
          do i________ = 1, size(x, 9)
           do i_______ = 1, size(x, 8)
            do i______ = 1, size(x, 7)
             do i_____ = 1, size(x, 6)
              do i____ = 1, size(x, 5)
               do i___ = 1, size(x, 4)
                do i__ = 1, size(x, 3)
                 do i_ = 1, size(x, 2)
                  do i = 1, size(x, 1)
                   res = res + x(i,i_,i__,i___,i____,i_____,i______,i_______,i________,i_________,i__________,i___________,i_______&
                       &_____,i_____________,i______________)
                  end do
                 end do
                end do
               end do
              end do
             end do
            end do
           end do
          end do
         end do
        end do
       end do
      end do
     end do
    end do

Impressive work!

[jvdp1]

Thanks for your review.

How complicated would it be to use more standard variable names such as i,j,k,l,m,n...? Maybe it is worth discussing whether we care about the style of auto-generated code.

I am not sure that I would call i,j,k,l,... standard variables. Anyway, I started this implementation by using i,j,k,l,.... But afterwards, I changed my idea because it means that it will reserve 15 characters (in these cases) for loop iterations. Also, it was easier for me to use additional "_" in fypp because it involves a simple loop.

Maybe @aradi has some suggestions regarding this issue.

I cannot find the place where you encoded the 132 line character limit. The generated code seems to obey this rule:

It can be changed in fypp by using the argument -l. Indeed the default is 132. Should we limit it to 120? Note that in the future I would expect that these files are generated by CMake, and would not be directly available on Github.

[ivan-pi]

I am not sure that I would call i,j,k,l,... standard variables. Anyway, I started this implementation by using i,j,k,l,.... But afterwards, I changed my idea because it means that it will reserve 15 characters (in these cases) for loop iterations. Also, it was easier for me to use additional "_" in fypp because it involves a simple loop.

I understand your reasoning. It is also easier to understand for the maintainers of the template file. I was only wondering whether we care about "style" (including variable name choice) in the auto-generated files.

[jvdp1]

I cannot find the place where you encoded the 132 line character limit. The generated code seems to obey this rule:

It can be changed in fypp by using the argument -l. Indeed the default is 132. Should we limit it to 120? Note that in the future I would expect that these files are generated by CMake, and would not be directly available on Github.

In the style guide of stdlib, it is mentioned that:

Line length should be limited to 80 characters and must not exceed 132.

So we could limit fypp to 80 characters, or use the default lenght which is 132.

[jvdp1]

@ivan-pi Following @aradi 's suggestion I kept the default lenght (132) for the auto-generated files.
I also replaced all the underscores by numbers. This is now much easier to read. Thank you for mentioning this issue!

[aradi]

As for the line length, yes fypp defaults to 132. You can override it with the -l option, but I am not sure, whether it is worth the effort. Most of the time, programmers will have to deal with the fypp-source file not with the generated one, so I think, one should impose the 80 character limit in on those.

As for the variable names: One shoud probably use number suffixes (e.g. i1, i2, etc.) instead of repeated underscores, as latter are very difficult to debug in my opinion. Something along the line:

#:def varsuffix(rank)
${str(rank)}$
#:enddef

#:def varlist(varname, listsize)
${",".join([varname + varsuffix(i) for i in range(1, listsize + 1)])}$
#:enddef

[...]

integer :: ${varlist("i", 10)}$

[...]

#:for fj in range(rank, 0, -1)
  do i${varsuffix(fj}$ = 1, size(x, dim=${fj}$)
#:endfor

[jvdp1]

As for the variable names: One shoud probably use number suffixes (e.g. i1, i2, etc.) instead of repeated underscores, as latter are very difficult to debug in my opinion. Something along the line:

Thank you @aradi for this suggestion. Using numbers instead of "_" should be easier to debug indeed. I will implement your suggestion in this fypp file.

[aradi]

I guess I am overseeing something trivial, but are the nested do loops really necessary? For example in mean_${rank}$_all_${k1}$_${k1}$(x) they seem to be just making a simple summation, where we could use simply sum() instead. We could then probably spare a few fypp-instructions. (According to my experience with pre-processing, each spared pre-processor instruction improves readability, even if it was a fypp-directive 😉 )

[jvdp1]

I guess I am overseeing something trivial, but are the nested do loops really necessary? For example in mean_${rank}$_all_${k1}$_${k1}$(x) they seem to be just making a simple summation, where we could use simply sum() instead. We could then probably spare a few fypp-instructions. (According to my experience with pre-processing, each spared pre-processor instruction improves readability, even if it was a fypp-directive )

I agree. There are indeed simple sums. I implemented loops to be in agreement with the other scenarios, and also because I observed that such loops might outperform sum. In addition, when computing the sum of an integer array, it is first converted to a double-precision array. I think that doing something like sum(real(x, dp)) may request a huge temporary array in some cases, while only a temporary scalar is needed when loops are used. Am I wrong?

[jvdp1]

Thank you @aradi for the suggestion of using number suffixes (e.g. i1, i2, etc.) instead of repeated underscores. It is indeed easier to debug. This suggestion is now implemented in the last commit.

@certik @milancurcic @ivan-pi @aradi

I decided to keep all sums in loops in mean_${rank}$_all_${k1}$_${k1}$(x) (instead of replacing them by sum(x). My reasoning is that their structure remained similar to the one of mean_${rank}$_all_${k1}$_dp$(x) for which I want to avoid something like sum(real(x, dp)) (which may imply a temporary array). If I am wrong, please correct me.

If you agree with the PR in this state, please approve it explicitely. Then I guess it may be merged.

[certik]

I have issues with committing autogenerated 30K lines long files into a git repository. The diff at some of your commits won't even show at GitHub because it's so long.

Why not to use this opportunity and do the right thing --- not commit autogenerated files, and rather build them as needed at the CI? I can help.

The other issue I have is that I think we should use sum instead of the long loops.

My last issue is that macOS builds fail. It's probably unrelated to this PR (#122), but we need to fix it.

[jvdp1]

I have issues with committing autogenerated 30K lines long files into a git repository. The diff at some of your commits won't even show at GitHub because it's so long.

Why not to use this opportunity and do the right thing --- not commit autogenerated files, and rather build them as needed at the CI? I can help.

I started to implement that in another branch. I can merge it in this branch (or I open another PR).

The other issue I have is that I think we should use sum instead of the long loops.

As discussed earlier in this thread, I avoided to use sum to avoid temporary arrays, especially 1) when the mean of integer arrays are computed (e.g., to avoid sum(real(x, dp))) and 2) when the mean along a dimension need to be computed (e.g., sum(x, 8)). There will be no issue for small arrays, but for large arrays, it might create memory problems. Also, my experience is that loops might be more (memory-)efficient for such scenarios. But I may be wrong on that and overlook some things.

[certik]

If the generated file is not committed to the repository, then I am fine with loops, as we can easily change that later.

[certik]

Can you merge with master to get #123 in? The tests should pass now.

[jvdp1]

Can you merge with master to get #123 in? The tests should pass now.

I will do that. Or should I first merge the master in my branch stat_mean_dev_fypp (with modified CMake files and Makefile.manual). The 2 large .f90 are now removed and auto-generated by CMake. I also modify the CI (but not for Windows).

[certik]

In that case I would suggest you create a fresh PR with clean commits on top of the master (no generated files are checked in).

[jvdp1]

In that case I would suggest you create a fresh PR with clean commits on top of the master (no generated files are checked in).

Good. I will do that.

[jvdp1]

I will close this PR in favor of #124, as suggested by @certik.
In additition to this PR, #124 supports the auto-generation of .f90 files by fypp.