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

Apply the modifications of https://github.com/libprima/prima/pull/135 to `Aeq`, `beq`, `Aineq`, and `bineq`? #146

Open zaikunzhang opened 8 months ago

zaikunzhang commented 8 months ago

Hi @nbelakovski ,

I have gone through https://github.com/libprima/prima/pull/135 and made some revisions to the algorithm_c.f90 files:

https://github.com/libprima/prima/commit/9880e58db0f648401a8e5ae701004e2c8ab7b456

It is mainly to

Do they seem OK to you?

In addition, I have got a question: Should we apply the modifications of PR 135 to Aeq, beq, Aineq, and bineq? See

https://github.com/libprima/prima/blob/439019fff6cc8afaece2d5a97ad695a0ba375d71/c/lincoa_c.f90#L92-L95 https://github.com/libprima/prima/blob/439019fff6cc8afaece2d5a97ad695a0ba375d71/c/cobyla_c.f90#L100-L103

zaikunzhang commented 8 months ago

BTW, does the following initialization set all integers in prima_options_t to 0 and all pointers to NULL? Is this guaranteed? Thanks.

    memset(options, 0, sizeof(prima_options_t));
nbelakovski commented 8 months ago

Do they seem OK to you?

A couple comments:

  1. You have a comment that reads See Sec. 9.7.1.3 and 15.5.2.13 of J3/24-007 (Fortran 2023 Interpretation Document). I think maybe we should reference the Fortran 2008 standard since the goal of the project is to be compatible with 2008, right? I just wouldn't want anyone to read this comment and walk away thinking that this code must be compiled with the 2023 standard.
  2. You have a comment that reads Deallocate variables not needed any more. Indeed, automatic allocation will take place at exit.. Did you mean automatic deallocation? If so, then (unless I misunderstand Fortran) automatic deallocation isn't considered a valid cleanup strategy and shouldn't be relied upon. It's true that if I write a C program like this one:
    int main(int argc, char * argv[]) {
      int *x = malloc(8);
      FILE *fp = open("somefile.txt", 'r');
      return 0;
    }

    then yes, x will be freed before the program exits (or I guess after it exits?) and the file fp will be closed and from a purely practical point of view it doesn't make any difference if I call free and close before return 0;. However as a "responsible person" the "right thing to do" is to call free and close before return 0;. The point I'm making here is that the second sentence in your comment, the one that starts with "Indeed," is unnecessary and can be removed.

  3. In lincoa_c.f90 the line that starts ! initialization to null because it implies ... was accidentally deleted. The whole comment is missing entirely in uobyqa_c.f90. One day we should make a unified interface in Fortran so we don't have to write almost the same code 5 times.

In addition, I have got a question: Should we apply the modifications of PR 135 to Aeq, beq, Aineq, and bineq?

I think maybe it's not necessary because we rely on m_eq and m_ineq to set the size of the corresponding matrices/vectors in Fortran, and if they're 0 (which is their default value), then that shouldn't cause any problems right? I haven't actually looked at how Fortran handles 0 length vectors or nx0 matrices, but I haven't seen any segfaults from this code yet so I just assumed it's fine.

BTW, does the following initialization set all integers in prima_options_t to 0 and all pointers to NULL? Is this guaranteed? Thanks.

Yes it sets all integers to 0 and all pointers to NULL and yes this is guaranteed.

zaikunzhang commented 8 months ago
  1. You have a comment that reads See Sec. 9.7.1.3 and 15.5.2.13 of J3/24-007 (Fortran 2023 Interpretation Document). I think maybe we should reference the Fortran 2008 standard since the goal of the project is to be compatible with 2008, right? I just wouldn't want anyone to read this comment and walk away thinking that this code must be compiled with the 2023 standard.

PRIMA aims to be compatible with F2008 and above. This means that the code compiles without any warning using any compiler with fstd=xyz (or something equivalent), where xyz >= 2008 (not only xyz = 2008). The code should not use any construct that does not exist in F2008, and should exploit the new features of F2008 whenever appropriate.

When referring to the Fortran standard for some feature, I always use the latest standard (F2023 as of today). If the standards are inconsistent about this feature, then PRIMA should not use it at all (I do not this has ever happened, as Fortran is super conservative about backward compatibility). Since the standards are consistent, referring to the latest one is better.

zaikunzhang commented 8 months ago

2. You have a comment that reads Deallocate variables not needed any more. Indeed, automatic allocation will take place at exit.. Did you mean automatic deallocation? If so, then (unless I misunderstand Fortran) automatic deallocation isn't considered a valid cleanup strategy and shouldn't be relied upon.

See https://fortran-lang.discourse.group/t/best-practice-deallocating-allocatable-arrays-explicitly-vs-implicitly.

This is a feature of modern Fortran that I avoid using. Another related one is automatic (re)allocation. I believe explicit allocation and deallocation are better, exactly due to the explicitness. In addition, explicit deallocation enables us to free the memory as soon as possible, which is beneficial if the memory under discussion is huge.

Thanks.

zaikunzhang commented 8 months ago

In lincoa_c.f90 the line that starts ! initialization to null because it implies ... was accidentally deleted. The whole comment is missing entirely in uobyqa_c.f90.

Fixed: https://github.com/libprima/prima/commit/1bdb6ea462e14eed11bb1d1c4330d7f297ffa2df

One day we should make a unified interface in Fortran so we don't have to write almost the same code 5 times.

I thought about it before. Currently, I have no plan for this.

nbelakovski commented 8 months ago
  1. You have a comment that reads Deallocate variables not needed any more. Indeed, automatic allocation will take place at exit.. Did you mean automatic deallocation? If so, then (unless I misunderstand Fortran) automatic deallocation isn't considered a valid cleanup strategy and shouldn't be relied upon.

See https://fortran-lang.discourse.group/t/best-practice-deallocating-allocatable-arrays-explicitly-vs-implicitly.

This is a feature of modern Fortran that I avoid using. Another related one is automatic (re)allocation. I believe explicit allocation and deallocation are better, exactly due to the explicitness. In addition, explicit deallocation enables us to free the memory as soon as possible, which is beneficial if the memory under discussion is huge.

Thanks.

I believe you misunderstood my comment. I was making two points

  1. You say automatic allocation will... and I wondered if this was a typo and perhaps you meant automatic DEallocation will ...?
  2. Since both you and I agree that explicit deallocation is better, the comment doesn't make a lot of sense. If you're choosing to explicitly deallocate variables, why would you tell the reader that they will be implicitly deallocated anyway? Perhaps you want to say something like "We prefer explicit deallocation even though automatic deallocation will take place at function exit"?