Miscelaneous updates (second batch) #18

talassio · 2019-03-01T16:55:06Z

Second (+10 commits, Jan 29 2019 to Feb 18 2019) mainly related to new printing proposal
* Write(out, ('...'))... is replaced by buildmsg/printmsg pair with
* Write(rec(i), Fmt=NNNNN)...
* Only uses options%out and not options%error. See note.
* No more Go To labels, except 100 for exit
* Comply with LAPACK interface description

Note these commits don't address possible issue with options%error i/o unit output, future commit will address issue.

Note this batch breaks STP

This commit addresses - Allocs and Deallocs have `stat` parameter and are handled gracefully Also some cosmetic changes are introduced - Most routines in `ral_nlls_workspace` now have a single exit address without returns - Where appropiate return from call to routines will handle `inform%status` - Propagated some inform%status/=0 - Added new error code: NLLS_ERROR_WRONG_INNER_METHOD = -15

This commit updates - NLLS_ITERATE to have single exit address 100 - inform%iter is reset correctly in w%first_call==1 - NLLS_inform struct initializes %iter to zero

Print level 1 proposal sketch

- new proposal for iteration log formated output - replaced all `write(out,...` with `buildmsg`/`printmsg` scheme - avoid extra calculations on last iteration of loop in `more_sorensen_ew` - possible standing issue with `svdstatus/=0`

- Add printing protocol to `ral_nlls_dtrs_double` routines - Remove unused elements in derived types: used in `ral_nlls_dtrs_double` routines` (out,error,print_level)

- Added one missing build/printmsg pair - Added inform%status monitoring to evaluate_model

- switch_to_quasi_newton routine expects for `w%hf_temp` to be defined: switch_to_quasi_newton_tests is updated to have it defined

- calls to `dsyevx` now comply with interface: The fix requires changes in two parts (1) allocate w%ew(1) in derived type `min_eig_symm_work` in `setup_workspace_min_eig_symm` regardless of the value of `options%subproblem_eig_fact` - `options%subproblem_eig_fact==T` -> w%ew(n) - `options%subproblem_eig_fact==F` -> w%ew(1) (2) allocate w%ew(n) in derived type `all_eig_symm_work` in `setup_workspace_all_eig_symm`, w%ew(n) is deallocated at exit of the subroutine. - add_matrices c=a+b has intent(inout) :: c - partially fixes issue with `regularization_solver` in the case where reg_oder==two, normd is not updated. Problem continues when reg_order/=two - updated 1line/it information with it==0 format, and added a new column printing the step length `w%normd` - added hidden option to force calling `min_eig_symm` without first calling `solve_spd_nocopy`. - Added new subroutine to add matrices C=A+B without aliasing call `add_matrices(w%A,B,n)`, replaces call of add_matrices(w%A,B,n,w%A) - valgrind on gfortran -O3 shows no leaks.

…nform%step=w%normd

1. Update example nlls_example2 to use type_of_method = 2 instead of illegal value 3 2. Check_options before entering nlls_iterate(), this routine for now only check for * print_level, and * type_of_method 3. Added Misc Error number -900 for illegal value of print_level

Replaces all constants with numeric values, `ral_nlls_workspaces` exports `wp` and error codes

tyronerees

CMakeLists.txt

The updated CMakeLists.txt should work for nagfor out-of-the-box too (although possibly not for profiling).

(& see comment in code)

example/C/nlls_example.c
(see comment in code)

example/C/nlls_example2.c
(see comment)

example/Fortran/Lanczos.f90
I'm happy with the changes here

example/Fortran/nlls_example.f90
(see comment)

example/Fortran/nlls_example2.f90
Print levels again. Here it seems to be set twice?

include/ral_nlls.h
Looks good

src/ral_nlls_double.f90
Looks good

ral_nlls_dtrs_double.f90
I'd like to ringfence this file -- possibly this will have to be changed in the NAG version of the code only, if changing this is necessary from your end. This is actually a dependency on another package (the routines here are lifted directly from Galahad), and I'm worried that if we edit this we won't be able to easily import any bug fixes and/or new features that are made in Galahad. @nimgould -- what do you think?

ral_nlls_internal.f90

(see comments in code)

src/ral_nlls_workspace.f90

(see comment in code)

tests/example_module.f90
(see comments in code)

Documentation
The docs are now out of date because of the printing changes. Could you update those to include the new control parameters, and to describe how they are used?

libRALFit/CMakeLists.txt

libRALFit/example/C/nlls_example.c

libRALFit/example/C/nlls_example2.c

libRALFit/example/Fortran/nlls_example.f90

libRALFit/example/Fortran/nlls_example2.f90

libRALFit/src/ral_nlls_internal.f90

libRALFit/test/example_module.f90

libRALFit/src/ral_nlls_workspaces.f90

* removed `options%error`, print scheme now uses only `options%out` * changed default print level to zero (no output)

libRALFit/test/nlls_c_test.c

libRALFit/src/ral_nlls_internal.f90

* Default print level is zero * Comments typos fixed * Miscelanea

codecov-io · 2019-04-01T09:41:52Z

Codecov Report

Merging #18 into master will decrease coverage by 6.95%.
The diff coverage is 76.23%.

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
- Coverage   90.99%   84.04%   -6.96%     
==========================================
  Files           3        4       +1     
  Lines        2255     2664     +409     
==========================================
+ Hits         2052     2239     +187     
- Misses        203      425     +222

Impacted Files	Coverage Δ
libRALFit/src/ral_nlls_internal.f90	`84.94% <ø> (-8.5%)`	⬇️
libRALFit/src/ral_nlls_printing.f90	`100% <100%> (ø)`
libRALFit/src/ral_nlls_ciface.f90	`81.21% <100%> (+0.29%)`	⬆️
libRALFit/src/ral_nlls_workspaces.f90	`78.21% <65.56%> (-10.45%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29ea3ee...9f33fbc. Read the comment docs.

* source file ral_nlls_dtrs_double.f90 is reverted to 7e2f4b6 * ral_nlls_internal reference calls to routines in `ral_nlls_dtrs_double` are updated.

* source file ral_nlls_dtrs_double.f90 is reverted to a2053e4, * ral_nlls_internal reference calls to routines in `ral_nlls_dtrs_double` are updated.

tyronerees · 2019-04-01T14:23:38Z

The changes to the codes look good -- thanks, Andrew!

These changes have dropped the test coverage percentage significantly. In particular, the tests don't hit the new printing routines you've written. Could you amend test/nlls_test.f90 to ensure that all the various branches of the printing code is hit when this is run? I write options%out to nlls_test.out and options%error to nlls_test_error.out there, and I change the print levels at various points throughout the file to ensure everything is hit at least once.

As discussed, we still need the docs to be updated before this can be merged in to master, as they should accurately describe the code. I see the you have added issue #22 to do this, which is great. Sphinx (https://docs.readthedocs.io/en/latest/intro/getting-started-with-sphinx.html) is pretty easy to change, but let me know if you have any trouble.

Update to RST/tex document files to reflect * new print level scheme * remove `options%error` unit number * add `inform%step` This should fix issue #22

* `nlls_test` now exersizes most of ral_nlls_printing module * bug fix in `print_options` * `default_options` now also includes `print_options` and `print_header` members

Docs where updated to describe options: * `print_options`, and * `print_header`

talassio · 2019-04-02T13:22:07Z

The changes to the codes look good -- thanks, Andrew!

These changes have dropped the test coverage percentage significantly. In particular, the tests don't hit the new printing routines you've written. Could you amend test/nlls_test.f90 to ensure that all the various branches of the printing code is hit when this is run? I write options%out to nlls_test.out and options%error to nlls_test_error.out there, and I change the print levels at various points throughout the file to ensure everything is hit at least once.

As discussed, we still need the docs to be updated before this can be merged in to master, as they should accurately describe the code. I see the you have added issue #22 to do this, which is great. Sphinx (https://docs.readthedocs.io/en/latest/intro/getting-started-with-sphinx.html) is pretty easy to change, but let me know if you have any trouble.

Updated nlls_test and now it covers all the printing module, coverage still does not hit target due to allocation checks in nlls_workspaces.

tyronerees · 2019-04-11T11:33:14Z

A few more relatively minor points I've found:

Docs
In the Fortran docs:

print_options is logical, but listed as integer
print_header is integer, but listed as logical

In all docs:

the description of print_header should read "prints the column header every print_header iterations when print_level > 1" (not have the value 30 hard-coded in)

Python interface

The python interface breaks with this update. The file src/ral_nlls_pyiface.c should be updated to match the current option list.

The python interface is not currently included in the test -- I've added Issue #30 to address this. I've also discovered that the python instructions don't work on all systems (as it needs the numpy headers to be in the path) -- I've addressed this with issue #29, which is now fixed and merged into master.

Note: part of the changes required to compile the documentation in Python 3 are specified in commit https://github.com/talassio/RALFit/commit/046c28bd859f7d7505cb88f5acee1db88f0e6bdf#commitcomment-33157683

Tests

Can the if statements before the calls to remove workspaces be inlined to increase test coverage? In particular, at:
call to remove_workspaces
call to remove_workspace_get_svd
call to remove_workspace_dogleg
call to remove_workspace_AINT_tr
call to remove_workspace_min_eig_symm
call to remove_workspace_max_eig
call to remove_workspace_galahad
deallocating w%ew
call to remove_workspace_all_eig_symm
status check on setup_workspace_min_eig_symm
call to remove_workspace_more_sorensen
call to remove_workspace_generate_scaling
The error return NLLS_ERROR_WRONG_INNER_METHOD is not checked in the tests

Some block-if statements in ral_nlls_workspaces.f90 where changed to logical-if statements.

Proposal to update `conf.py` to work under Python 3.x

add `stat=` to dealloc call

Andrew Sajo and others added 30 commits January 29, 2019 17:07

more_sorensen_noew single exit address

4162c3e

USE/ONLY

31fa32e

Single exit address

1c93bd3

This commit updates - NLLS_ITERATE to have single exit address 100 - inform%iter is reset correctly in w%first_call==1 - NLLS_inform struct initializes %iter to zero

Printing Module

e4bd2fb

update to CMAKE to include printing module

71bdae5

Printing implementation proposal

be17388

Print level 1 proposal sketch

PrintMSG and BuildMSG

a5d9f78

- new proposal for iteration log formated output - replaced all `write(out,...` with `buildmsg`/`printmsg` scheme - avoid extra calculations on last iteration of loop in `more_sorensen_ew` - possible standing issue with `svdstatus/=0`

PrintMSG and BuildMSG (cont.)

3c79720

- Add printing protocol to `ral_nlls_dtrs_double` routines - Remove unused elements in derived types: used in `ral_nlls_dtrs_double` routines` (out,error,print_level)

Missing printmsg

bd8c1b5

- Added one missing build/printmsg pair - Added inform%status monitoring to evaluate_model

Armor (test)

bbc66c5

- switch_to_quasi_newton routine expects for `w%hf_temp` to be defined: switch_to_quasi_newton_tests is updated to have it defined

LAPACK interface check fails for w%ew(1) instead of w%ew(n)

2f438ae

cosmetic change for pre-NAG-patch of DLAMCH removal

46be524

Do not iterate if x0 is solution point!

eb995ab

Init workspace%normd = huge

caaf7c7

Add print_bye routine to print exit summary

012b15b

add new step=w%normd element to nlls_inform

eef0dfd

move exit summary/error printing to print_bye and added updates for i…

0c01d35

…nform%step=w%normd

Fix duplicate printing of same exit error

b2c32df

Check Options

4eae62f

1. Update example nlls_example2 to use type_of_method = 2 instead of illegal value 3 2. Check_options before entering nlls_iterate(), this routine for now only check for * print_level, and * type_of_method 3. Added Misc Error number -900 for illegal value of print_level

Constants (partial replacement)

85119b1

Replaces all constants with numeric values, `ral_nlls_workspaces` exports `wp` and error codes

require v3 of CMake

064a356

put langage into the project

3389429

remove half-baked mkl support

3336595

remove commented out hsl files

bfa8f5c

Tidy up flags

1b9e509

Fix project types supported

a80104e

Move blas section

316c3db

Add Fortran coverage, and remove need for c++ compiler

e72ecc6

talassio self-assigned this Mar 28, 2019

talassio added the enhancement label Mar 28, 2019

tyronerees requested changes Mar 29, 2019

View reviewed changes

Update to options

8772c3b

* removed `options%error`, print scheme now uses only `options%out` * changed default print level to zero (no output)

talassio commented Mar 29, 2019

View reviewed changes

libRALFit/test/nlls_c_test.c Outdated Show resolved Hide resolved

talassio commented Mar 29, 2019

View reviewed changes

libRALFit/src/ral_nlls_internal.f90 Outdated Show resolved Hide resolved

Andrew Sajo added 2 commits April 1, 2019 10:18

Merge branch 'update_cmake' of https://github.com/ralna/RALFit

887e095

Resolves requested changes #18

87db54a

* Default print level is zero * Comments typos fixed * Miscelanea

talassio requested a review from tyronerees April 1, 2019 09:44

Andrew Sajo added 3 commits April 1, 2019 14:33

RING FENCE for RAL_NLLS_DTRS_DOUBLE

1b6f44f

* source file ral_nlls_dtrs_double.f90 is reverted to 7e2f4b6 * ral_nlls_internal reference calls to routines in `ral_nlls_dtrs_double` are updated.

RING FENCE for RAL_NLLS_DTRS_DOUBLE

9e61973

* source file ral_nlls_dtrs_double.f90 is reverted to a2053e4, * ral_nlls_internal reference calls to routines in `ral_nlls_dtrs_double` are updated.

Merge branch 'master' of https://github.com/talassio/RALFit

c42447a

Andrew Sajo added 4 commits April 2, 2019 12:38

fix issue #22

1c069cd

Update to RST/tex document files to reflect * new print level scheme * remove `options%error` unit number * add `inform%step` This should fix issue #22

Update printing coverage

7ae8109

* `nlls_test` now exersizes most of ral_nlls_printing module * bug fix in `print_options` * `default_options` now also includes `print_options` and `print_header` members

Update to DOCs

9a65eb7

Docs where updated to describe options: * `print_options`, and * `print_header`

Increase coverage of print levels 4 & 5

2fdbe9b

tyronerees mentioned this pull request Apr 2, 2019

singular values #17

Closed

Andrew Sajo added 4 commits April 11, 2019 13:16

Block-if moved to logical-if statement

ffeb26f

Some block-if statements in ral_nlls_workspaces.f90 where changed to logical-if statements.

Fix typos to docs

2ab4a40

Update to Python C interface

cb72c20

Python 3.x changes to documentation generator

046c28b

Proposal to update `conf.py` to work under Python 3.x

talassio mentioned this pull request Apr 12, 2019

Python tests #30

Closed

Andrew Sajo added 2 commits April 12, 2019 16:08

Add test for error NLLS_ERROR_WRONG_INNER_METHOD

091b3fd

dealloc status

9f33fbc

add `stat=` to dealloc call

tyronerees approved these changes May 8, 2019

View reviewed changes

tyronerees merged commit 2cbb17c into ralna:master May 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscelaneous updates (second batch) #18

Miscelaneous updates (second batch) #18

talassio commented Mar 1, 2019 •

edited

Loading

tyronerees left a comment •

edited

Loading

codecov-io commented Apr 1, 2019 •

edited

Loading

tyronerees commented Apr 1, 2019

talassio commented Apr 2, 2019

tyronerees commented Apr 11, 2019 •

edited by talassio

Loading

Miscelaneous updates (second batch) #18

Miscelaneous updates (second batch) #18

Conversation

talassio commented Mar 1, 2019 • edited Loading

tyronerees left a comment • edited Loading

Choose a reason for hiding this comment

codecov-io commented Apr 1, 2019 • edited Loading

Codecov Report

tyronerees commented Apr 1, 2019

talassio commented Apr 2, 2019

tyronerees commented Apr 11, 2019 • edited by talassio Loading

talassio commented Mar 1, 2019 •

edited

Loading

tyronerees left a comment •

edited

Loading

codecov-io commented Apr 1, 2019 •

edited

Loading

tyronerees commented Apr 11, 2019 •

edited by talassio

Loading