Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Finite-Diffs (FD Branch) to RALFit #121

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open

Add Finite-Diffs (FD Branch) to RALFit #121

wants to merge 32 commits into from

Conversation

as-amd
Copy link
Collaborator

@as-amd as-amd commented Aug 14, 2024

This branch enables

* Approximate Jacobian matrix using FD
* Check user-supplied Jacobian using FD


* Update doc
* Update UT
* Add examples

Andrew Sajo and others added 19 commits July 10, 2024 17:24
 * Adds support for FD in eval_J
 * Tweaks examples
 * Add copyright notice banners

Change-Id: I2ea721708a323c840f08af2d254b9c68e5f259a7
Change-Id: Ia05dbe4d07d00f66b00fd7555497a3b3d415f34f
Add module to parametrize data types (int, real) in preparation
to build LP64/ILP64 and float/double variants

It also tweaks some of the examples

Change-Id: I8440c86be379839677bf65f5163e9d38cd17bb1f
Also fixes nlls_example2.c bug

Change-Id: Iec2ec4983eae41e229472d2bf902258a0a229d1d
Change-Id: I45d8f565b07c4c7330317061db53ed09941f5e3e
 * jacobian_setup,
 * jacobian_calc and
 * jacobian_free
 * example to show how to call this framework instead of using the
   solver's internal mecanism

Change-Id: I4eb24ce1cf67a78dceac541066f509657ef3d202
Example to illustrate how to use RALFit FD
machinery to estimate a Jacobian matrix outside of
the solver

Change-Id: I34b81558e6375c8973f6988a65f7708354f3b391
Update and tidy examples

Change-Id: I094948f5705f836c7d2aa4905b6081ed6f0c6c06
Added new extended examples
Fixed bug in weight size passed in linesearch

Change-Id: Ia58f6c8bf74e7da3999b2c69e48fed874c95d112
 * tweak examples
 * UT fd checker
 * UT fd solver

Change-Id: I8f8d4fb8674f7058a453400b1d66449f0ea08c4d
Change-Id: I223cb5906f8853b7b346bef5a7ef18f2304a772e
Change-Id: Icdbf3b11671b17cc9bb2248a3e60ba07d2042630
Fixes ifort compilation error

Change-Id: Ib6ece887276e4a2700c79d26d224182edf0d3ec4
Still need to move common chunks to `../common`

Change-Id: Id611ab225488814649606de88bc3085c34f20689
Change-Id: Ibf48fa781dda37c4adc9cd581da0512dbc5177c3
* update the docs for python 3 and rtd

* Fix warnings, including c issues

* fix typos and c signature

* Add a docs readme

* Fix python warnings/errors
 * Typos
 * Removal of trailing blanks

Change-Id: I85304c042b21728edfc544b12fa93b9a0d76eac7
Change-Id: I2f3d96e9af7d878b40a54bc05197fc94a2fae070
Copy link
Member

@tyronerees tyronerees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for this! I've dotted a few comments through the code.

I get a test failure when I run the code locally:
image

I've got a few more things to check, but I thought it's worth feeding this back from the first pass.

Also note that @AndrewLister-STFC fixed the python installation for us -- that's now merged into master (and doesn't conflict with anything here)

@@ -1,10 +1,18 @@
# Copyright (c) 2016, The Science and Technology Facilities Council (STFC)
# All rights reserved.
# Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved.
# CMake project file for libral_nlls
cmake_minimum_required (VERSION 3.1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be bumped -- Fortran_PREPROCESS was added in 3.18, so that should (at least) be the minimum.

Comment on lines 9 to 14
# Request to run pre-processor as well
if(NOT WIN32)
set(CMAKE_Fortran_FLAGS -cpp)
else()
set(CMAKE_Fortran_PREPROCESS ON)
endif()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does CMAKE_Fortran_PREPROCESS do? Is it enough to set this for all architectures -- would that set the right flag for us?

Copy link
Collaborator Author

@as-amd as-amd Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CMake flag calls CPP on .F90 files and consumes any C-preprocessor directive in Fortran 90 files, so #include (et al) work. We can certainly enable it for all architectures.

@@ -202,6 +202,9 @@ It must have the following signature:

:param status: |eval_J_status|

If user does not provide the call-back it may be substituted by ``NULL`` and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this needs to be here as well. I think I'd change must -> may in the description before this, and possibly mention changing to NULL there (although I'm not sure this is needed). This is all said in the calling sequence, and I think that's enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in C and Fortran howtouse.rst

@@ -229,6 +232,8 @@ It must have the following signature:

:param status: |eval_Hf_status|

If user does not provide the call-back it may be substituted by ``NULL``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here: changing must -> may is probably enough.

libRALFit/doc/C/howtouse.rst Outdated Show resolved Hide resolved
libRALFit/test/nlls_test.f90 Outdated Show resolved Hide resolved
libRALFit/test/nlls_test.f90 Outdated Show resolved Hide resolved
libRALFit/test/nlls_test.f90 Outdated Show resolved Hide resolved
libRALFit/test/nlls_test.f90 Outdated Show resolved Hide resolved
Comment on lines +19 to +33
#ifdef SINGLE_PRECISION
Integer, Parameter, Public :: wp = lp
Integer, Parameter, Public :: ral_c_real = c_float
#else
Integer, Parameter, Public :: wp = np
Integer, Parameter, Public :: ral_c_real = c_double
#endif

#ifdef BUILD_ILP64
! To build ilp64 also the default integer kind must be set to 8 bytes
Integer, Parameter, Public :: ral_c_int = c_int64_t
#else
Integer, Parameter, Public :: ral_c_int = c_int
#endif

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something, but I'm not sure this is enough. There are lots of calls to lapack and blas in the codes, and the calls to d___ will have to change to s___

It might also be worth documenting how to switch precisions in the installation section of the docs.

Copy link
Collaborator Author

@as-amd as-amd Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad you brought this up, these changes don't introduce support for single precision it only tries to gather into a single place wp and it's C pair. The support for single precision PR is much, much more extensive! My next PR addresses this topic and will be properly documented.

as-amd and others added 8 commits September 18, 2024 11:29
Change-Id: Ic63a9aeacf759fc1e67c05955233b9b92f0a0b3f
Remove duplicated text and include from common file

Change-Id: I984b3a7b33239bf282f5395be14d9691f186c790
On some numerical lib/hardware combinations some FD tests where failing.
Tolerances are tweaked to account for this variability, also
some test with starting iterate are updated to be "closer" to the
required solution.

Change-Id: I33d97c1591c4c3a509f2873a662a6f546238fc01
On some numerical lib/hardware combinations some FD tests where failing.
Tolerances are tweaked to account for this variability, also
some test with starting iterate are updated to be "closer" to the
required solution.

Change-Id: I33d97c1591c4c3a509f2873a662a6f546238fc01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants