libprima / prima

PRIMA is a package for solving general nonlinear optimization problems without using derivatives. It provides the reference implementation for Powell's derivative-free optimization methods, i.e., COBYLA, UOBYQA, NEWUOA, BOBYQA, and LINCOA. PRIMA means Reference Implementation for Powell's methods with Modernization and Amelioration, P for Powell.
http://libprima.net
BSD 3-Clause "New" or "Revised" License
304 stars 40 forks source link

Drop array size hints #76

Closed jschueller closed 1 year ago

jschueller commented 1 year ago

see https://github.com/JuliaInterop/Clang.jl/discussions/443

github-actions[bot] commented 1 year ago

@check-spelling-bot Report

:red_circle: Please review

See the :open_file_folder: files view, the :scroll:action log or :angel: SARIF report for details.

Unrecognized words (1)

libclang

To accept :heavy_check_mark: these unrecognized words as correct, run the following commands ... in a clone of the [git@github.com:jschueller/prima.git](https://github.com/jschueller/prima.git) repository on the `hint` branch ([:information_source: how do I use this?]( https://github.com/check-spelling/check-spelling/wiki/Accepting-Suggestions)): ``` sh curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/main/apply.pl' | perl - 'https://github.com/libprima/prima/actions/runs/6205842038/attempts/1' ```
If the flagged items are :exploding_head: false positives If items relate to a ... * binary file (or some other file you wouldn't want to check at all). Please add a file path to the `excludes.txt` file matching the containing file. File paths are Perl 5 Regular Expressions - you can [test]( https://www.regexplanet.com/advanced/perl/) yours before committing to verify it will match your files. `^` refers to the file's path from the root of the repository, so `^README\.md$` would exclude [README.md]( ../tree/HEAD/README.md) (on whichever branch you're using). * well-formed pattern. If you can write a [pattern](https://github.com/check-spelling/check-spelling/wiki/Configuration-Examples:-patterns) that would match it, try adding it to the `patterns.txt` file. Patterns are Perl 5 Regular Expressions - you can [test]( https://www.regexplanet.com/advanced/perl/) yours before committing to verify it will match your lines. Note that patterns can't match multiline strings.
zaikunzhang commented 1 year ago
  1. Let me reconfirm, this is not VLA, right? If yes, then it is something better retained because it is quite informative and user-friendly.

  2. However, due to the limitation of Clang.jl at the moment, let us compromise as follows.

    • Make minimal changes. Do we need to change both prima.h and prima.c? According to the discussion at Clang.jl, I guess only prima.h needs modification, right? If yes, let us modify prima.h only. In addition, add some comment in prima.h such as
      Unlike prima.c, the arrays here do not contain hint sizes. This is because we want to use Clang.jl to generate Julia wrappers, but Clang.jl does not support hint sizes as of 20230916 ( https://github.com/JuliaInterop/Clang.jl/discussions/443 ). Hint sizes will be added once Clang.jl supports ( https://github.com/JuliaInterop/Clang.jl/issues/444 ).
    • Restore the hint sizes when Clang.jl supports, as mentioned in the above-proposed comments.

What do you think?

Thanks.

Gnimuc commented 1 year ago

This might be off-topic, but as C compilers always treat arrays as pointers in a function prototype and do not perform any size check, and I don't find any size check in the c wrapper, does it happen on the Fortran side?

zaikunzhang commented 1 year ago

This might be off-topic, but as C compilers always treat arrays as pointers in a function prototype and do not perform any size check, and I don't find any size check in the c wrapper, does it happen on the Fortran side?

Hi @Gnimuc ,

The c-binding Fortran subroutines (e.g., prima/c/cobyla_c.f90) do not do any size check, although the sizes are included in the declaration of the arrays (compilers will not check them).

If we go deeper into the Fortran implementation itself (e.g., prima/fortran/cobyla.f90), arrays are passed as "assumed-shape arrays". The sizes of those arrays are not included in the declaration but can be extracted from the arrays (I guess they are implemented as structures at a lower level), and the Fortran implementation of PRIMA does check that the sizes of the inputs are correct --- note that it is done manually within the code at runtime, not by the compiler.

Thanks.

jschueller commented 1 year ago

I would remove them completely in both .h and .c for consistency, they dont check anything (I think fortran doesnt check much either). Checks can be done easily in the higher level languages though, see how I added asserts in the Python layer.

Gnimuc commented 1 year ago

This might be off-topic, but as C compilers always treat arrays as pointers in a function prototype and do not perform any size check, and I don't find any size check in the c wrapper, does it happen on the Fortran side?

Hi @Gnimuc ,

The c-binding Fortran subroutines (e.g., prima/c/cobyla_c.f90) do not do any size check, although the sizes are included in the declaration of the arrays (compilers will not check them).

If we go deeper into the Fortran implementation itself (e.g., prima/fortran/cobyla.f90), arrays are passed as "assumed-shape arrays". The sizes of those arrays are not included in the declaration but can be extracted from the arrays (I guess they are implemented as structures at a lower level), and the Fortran implementation of PRIMA does check that the sizes of the inputs are correct.

Thanks.

thanks for the explanation. makes sense to me now.

zaikunzhang commented 1 year ago

I would remove them completely in both .h and .c for consistency, they dont check anything (I think fortran doesnt check much either). Checks can be done easily in the higher level languages though, see how I added asserts in the Python layer.

I am a bit hesitant. I was thinking that the sizes were quite informative to users. I did not think about the size check.

So my proposal was to remove those in prima.h for the moment and restore them when possible. But I do not know the recent common / best practices in C.

If you still think it is better to remove those in prima.c after rethinking, I will not disagree.

Thanks.

zaikunzhang commented 1 year ago

and the Fortran implementation of PRIMA does check that the sizes of the inputs are correct.

note that the check is done manually within the code at runtime, not by the compiler.

jschueller commented 1 year ago

yes, lets remove it everywhere