Refactor

[jacobwilliams]

Fixed to free source conversion:

• convert to lower case
• added subroutine names to END
• fixed issues where x(1) was being used instead of x(*) for dummy arguments
• declare all variables and add implicit none
• replaced float with real
• replaced double precision literals in data statements with parameters computed for rk kind
• restructuring and elimination of gotos and line numbers
• replaced old style logical operators with modern
• updated makefile

See also: #17

[certik]

Thanks a lot @jacobwilliams for working on this.

I agree with the changes as described in the commit. But I would now like to review them as actually implemented in the code.

Do you by any chance have individual commits for these changes?

It's very hard to review, because all I can see is fixed form on one side and a completely new file (free form) on the other side, and I can't easily see the actual changes such as goto elimination. It is quite easy to make a mistake and we need to check that no mistake was made.

It would be much easier to review if there were individual commits for each change that you described above.

The other thing that we have to do is to benchmark it, to ensure we have not slowed it down by a mistake.

[certik]

I suspect the hardest change is fixed form -> free form, so if we do just that change separately, the diff might become very clean and easy to review.

[jacobwilliams]

OK I added the intermediate commits to a parallel branch and merged that in. So you should see them.

Note: I use the SPAG tool to do the fixed to free conversion.

The ab13c44 commit was what I did manually, everything else was automated. There are a couple extra commits there for files that got in there by accident.

[certik]

Awesome, thanks a lot, I can now see the manual commit (ab13c44) and will review it. It's now nice and clean. Thank you!

[zoziha]

It looks cool, and the free format is always more welcome! If we want to confirm that the performance test has not been slowed down, what should we do?

[certik]

So I have pushed the original branch into my fork for backup purposes: https://github.com/certik/fftpack/tree/refactor. I have forced pushed into this PR, I kept the commits mostly, but made the following changes:

• Got rid of the original huge commit which was now a duplicate and a merge; makes the PR and git repository smaller
• Two files were committed as part of the SPAG commit that were first modified in the "lowercase" commit and then deleted; I have removed them already in the SPAG commit

I will continue reviewing this.

[certik]

This change is wrong. It should be:

       real(rk),parameter :: tpi = acos(-1.0_rk) * 2.0_rk ! 2 * pi

[certik]

I fixed this below...

[jacobwilliams]

oops

[certik]

Ok. I spent about half an hour reviewing the manual commit. I spotted one mistake, see above.

I can't promise that everything else is correct, I tried to do my best, but the change is just huge.

Overall I think this looks good. I don't like the new naming convention of Wa. It should be either WA or wa. Same with CC or cc, but not Cc, that just looks ugly. We can fix that in future commits.

We still have to benchmark this. To do that, we have to have some examples that call this for larger arrays and check the speed.

[certik]

Here is how to benchmark.

Master:

$ fpm run --example '*' --profile=release --flag="-march=native -ffast-math"
 Initializing
 Forward
 Backward
 Done

 Error:    1.5543122344752192E-015
 Init time:   0.55384199999999995     
 Forward time:    1.7490780000000001     
 Backward time:    1.6694180000000003     

This PR as of now:

$ fpm run --example '*' --profile=release --flag="-march=native -ffast-math"
 Initializing
 Forward
 Backward
 Done

 Error:    1.7763568394002505E-015
 Init time:   0.53533500000000001     
 Forward time:    1.8730339999999999     
 Backward time:    1.7849739999999996     

The timings vary on my computer, I took the best timings I was able to get. Overall I don't see a significant slowdown, but I would have to benchmark more to be sure. Overall good news.

[certik]

Ok, so I am leaning towards +1 to merge as it is now, however given the fact that I was able to find a mistake like the above, I worry there are more.

@hsnyder, @jacobwilliams, @zoziha and others, would you please mind going over the patches in this PR very carefully also and try to see any mistakes? It is much better to spot such mistakes now than not noticing.

Also the commit "Initial fixed to free processing by SPAG" has to be carefully checked, I just realized SPAG did quite a lot of changes beyond just fixed-form -> free-form. The changes are good in their nature (arithmetic ifs -> if/then/else), but we have to check for mistakes.

[zoziha]

It is indeed a huge refactoring. Apart from the code business logic, I don't seem to see any obvious omissions.

[jacobwilliams]

Yep i can review also when i get some time. I have use SPAG in the past, and have never seen it break anything, actually it works amazingly well... but that doesn't mean it isn't possible that it could have broken something. This is also where comprehensive unit tests with code coverage would help.

[jacobwilliams]

And yes, there are other cosmetic things to be done. I figure we get the current state reviewed, and then can work on those after that.

[hsnyder]

I will review this carefully as well, but it may take me a few days to get through it.

[certik]

No rush. Let's not merge any other PR until this is done and let's not push any more commits into this PR, unless there is a bug that we find.

Let's take a few days to carefully review this. It's worth it.

Thank you all!

[zoziha]

I checked it again, and the specific business is really hard to see. However, I think these changes have passed the CI unit test and there should be no obvious problems. If there are, we can wait for subsequent updates.

I think this PR can be passed.

[milancurcic]

Pending one conflicting file, can this PR go forward @certik @hsnyder @jacobwilliams?

I did not review specific code changes, but the description in the PR looks good and I trust @jacobwilliams with this, FWIW.

[zoziha]

I need a little time to update the code later.

[zoziha]

I agree to pass :)
Thanks to @jacobwilliams 's modernization work on fftpack, I feel that fftpack has a new lease of life, after many years of hiatus.

In the Chinese community, many students are using netlib's fftpack as assigned by their tutors, but they are new to Fortran. Due to the stricter checking of modern Fortran compilers, the mismatch of parameters will cause the compilation to fail.

[jacobwilliams]

I'm reviewing now...

[jacobwilliams]

OK, I reviewed my manual interventions. Saw the bug that @certik fixed, and also found another sign error, which I now see that @zoziha already fixed in db5967d (the comment was wrong in the original code).

I'm OK to merge. There's always room for more unit testing.

[hsnyder]

Sorry for the delay on my part. I'm reviewing today

[hsnyder]

All the tests are failing for me (fpm 0.5.0, gfortran 10.3, FreeBSD 13.0). The error message is some variation on Index '9' of dimension 1 of array 'wsave' above upper bound of 1 (same for all tests except for the exact index). Has anyone else encountered this?

[zoziha]

@hsnyder You may have used the main branch to compile the code. Due to the stricter defaults of modern Fortran compilers, so are the default compiler parameters (-fcheck=bounds in -Wall -Wextra -Wimplicit-interface -fPIC -fmax-errors=1 -g -fcheck=bounds -fcheck=array-temps -fbacktrace -fcoarray=single on gfortran) of fpm. Mismatched parameters will cause errors. You can use fpm test --flag "-O2" to get it working correctly.

Mismatches between actual and dummy argument lists in a single file are now rejected with an error. Use the new option -fallow-argument-mismatch to turn these errors into warnings; this option is implied with -std=legacy. -Wargument-mismatch has been removed. (see GCC 10 Release Series Changes, New Features, and Fixes)

Of course @jacobwilliams's modernized refactor branch, has solved this problem replacing (1) with (*).

[certik]

I get the same tests errors in master as reported, but no test errors in this PR. I rerun benchmarks again, I don't see any slowdown using fpm run --example '*' --profile=release --flag="-march=native -ffast-math" in this PR versus in main (I tested on Apple M1 with GFortran 11.0.1 from conda-forge).

So I am fine with merging as is.

As I said, I don't like changing CC to Cc and such. But given how long it took to review (over 4 months...), let's just get this in, and improve further once merged.

[hsnyder]

Thanks @zoziha for pointing out my (silly) mistake. I agree with @certik, let's get this in.

[certik]

Ok, so since @zoziha, @certik, @milancurcic and @hsnyder approved it, and @jacobwilliams also approved it, I am going to go ahead and merge.