-
Notifications
You must be signed in to change notification settings - Fork 42
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
Remove allocations for xl, xu, nlconstr0 in C interface #135
Conversation
if ((info == 0) && use_constr) | ||
{ | ||
// reuse or (re)allocate nlconstr | ||
if (result->_m_nlcon != problem->m_nlcon) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular if statement relied on the fact that result->_m_nlcon
would be 0 after the call to prima_init_result
. I moved it into prima_init_result
and relied on the value of problem->m_nlcon
to allocate it.
Hi @nbelakovski , two quick questions:
Thank you. |
It is the spirit of PRIMA to avoid requiring the user to do things like
This is unnatural and unfriendly. If this is inevitable, then we should add another layer to enable users to do BTW, excuse me for my ignorance about C, could you remind me why it is an arrow instead of a dot? N.B.: PRIMA tries to provide the most natural and friendly interface to users and handles everything else under the hood. Otherwise, PRIMA would not exist in the first place. Thank you. |
Fortunately no.
From the 2003 standard: Also section 13.7.88 indicates that using Link: https://wg5-fortran.org/N1601-N1650/N1601.pdf
The arrow is used when the left side is a pointer. An example: prima_problem_t problem;
problem.f0 = 5;
prima_problem_t *problem_ptr = &problem; // problem_ptr is of type pointer and points to problem
printf("%g\n", problem_ptr->f0); // prints 5
OK I have an idea, the options struct could look like this (using rhobeg as an example): typedef struct {
double rhobeg;
double* _rhobeg; // internal - do not use
} prima_options_t;
|
Thank you for the explanation.
Good. What about Thanks. |
I guess the following code will not compile or work as expected if Always keep in mind that
BTW, what does Thanks. |
I was able to make a test program work with a different value of https://godbolt.org/z/aKdWjzYo7 Basically it works like this: ! Compulsory arguments
type(C_PTR), value :: array_of_double_ptr
integer(C_INT), value :: num_elements
! Local variables
real(C_DOUBLE), pointer :: array_of_double_ptr_loc_intermediate(:)
real(RP), allocatable :: array_of_double_ptr_loc(:)
! Convert
if (C_ASSOCIATED(array_of_double_ptr)) then
call C_F_POINTER(array_of_double_ptr, array_of_double_ptr_loc_intermediate, shape=[num_elements])
allocate(array_of_double_ptr_loc(num_elements))
array_of_double_ptr_loc = real(array_of_double_ptr_loc_intermediate, kind(array_of_double_ptr_loc))
end if There's another way to do it with pointers if necessary, but it requires a second intermediate variable. Fortunately a variable marked
From the documentation:
Basically the brackets turn it into an array. I tried without the brackets and it complained about the variable not having the correct rank.
Usually a leading underscore is understood to mean that something is private. Plus I don't like the |
This matches my imagination. Let us use
I checked this on Internet. Understood. Let us follow this convention.
I will be happy to change if there is a better option. Thank you. |
Of course, I was only using allocate for the examples. Should this PR make the change for all the optional variables, or just the 4 I started with? We can also make an issue to fix the rest of them in a separate PR. |
What are the other variables? I guess the motivation for these four variably is that they are vectors, so allocation is needed somewhere. For scalar variables, what is the motivation? I have no objection, but I hope to understand the situation better. If the modification leads to a better (more friendly) interface or better (cleaner or more consistent) code, then of course we should do it. It is fine to handle the other variables on a separate PR, but I hope the delay would not be long, and I hope the C API will stabilize soon. Thank you. |
The other variables currently exposed are
There are of course other optional variables in the various algorithms that are not currently exposed, like honour_x0, eta, gamma, and others. For variables like maxfun and npt, we currently have logic within the C interface to initialize those, but this logic is duplicated as compared to what's in Fortran. It would be nice if C didn't duplicate any initialization code. The motivation really isn't related to whether or not the variables are vectors or scalars. The vector quantities were initially motivated by wanting to reduce unnecessary memory allocations, but overall both types would benefit from the ability to mark them as null in order to indicate to Fortran that they are "not present." |
I don't necessarily disagree. We can do it. However, duplicating the initialization in C is not necessarily a bad thing: if a pure C implementation will be done in the future, then it will need an interface that "duplicates (implements) the initialization". Indeed, the MATLAB interface of PRIMA and the Python interface of PDFO both implement the initialization. Indeed, they do much more, including problem cleaning, pre-processing, and refining, all of which are done very carefully. Obviously, these things are much easier to do in MATLAB/Python than in Fortran (or C). In comparison, the initialization in Fortran is relatively "primitive". I hope the Python interface for PRIMA will be similar to the MATLAB interface for PRIMA and the Python interface for PDFO. |
How to test similar things systematically? We should include such tests in our CI. See #129. |
If the initialization only involved default values, then there wouldn't be very much to discuss - it would be trivial to implement in any language and not too difficult to copy the dozen or so optional variables that we have. However, I've noticed that some variables have more complicated initialization routine, like rhobeg and rhoend, for example.
eta1 and eta2 have similar logic where they are initialized together. Right now in the C interface we have no way of detecting that rhoend was provided but rhobeg was not. I don't see this applying to other variables, so I think the logical course would be to implement the |
78760c3
to
2dd80c5
Compare
It seems nvfortran does not like my latest approach. I am investigating. |
Excuse me, @nbelakovski , it is a bit unclear to me what is the issue here. Do you mean you will implement the same initialization in the C interface, given that we have agreed on your proposed way of indicating the absence of If the answer to the above question is yes, then wouldn't it contradict the following idea? According to this idea, shouldn't we just read the values of
Thanks. |
No, I was pointing out that the C interface as currently designed (i.e in the This seems like a problem to me, and given my statement that I think C should be a 'dumb' pass-through, I think the way to solve this is to use that approach with I've investigated the nvfortran issue. The issue seems to come from the |
You are right. This setting is wrong.
Yes. Agreed. |
Thank you @nbelakovski for the investigation.
I see. It will not surprise me if we have caught another compiler bug. It would be better if we can submit a minimal example to nvfortran and get a confirmation that it is their bug.
The second approach sounds better unless it is much more complicated to implement. |
It looks like someone has run into this before: https://forums.developer.nvidia.com/t/bug-in-nvfortran-with-mchkptr-for-unallocated-optional-arguments/223220 They claim it's correct behavior, but they understand that this is a valid use case and offered to make a second flag to allow this sort of behavior while -Mchkptr is used. It doesn't seem like there was any follow up. I'll make a topic to follow up. In the meantime, I dug around, and it seems like it might technically be possible to remove a flag from within CMake for a specific target, but there's no standard-conforming way to do it, and I think it would be best to just remove the chkptr flag from CMake. If we need it, we can make a separate github action that only builds and tests the Fortran code and not the C interface. Edit: nvidia forum topic to revive the discussion around Mchkptr suboption https://forums.developer.nvidia.com/t/mchkptr-doesnt-allow-passing-null-optional-values/278293 |
2dd80c5
to
9d124a8
Compare
OK this PR is good to go @zaikunzhang |
Thank you @nbelakovski . I have made some comments. Can you see them (I am not sure)? Do not do a force push before checking all the comments, or they will disappear. |
No, I didn't see them, I'm sorry I didn't know force-pushing removed comments. Could you please re-comment? |
What about now? @nbelakovski |
9d124a8
to
995c330
Compare
But this substantially complicates the code. In addition, the code about f0 is wrong - check the code about f0 and nlconstr0 in cobyla.f90 (I am on cellphone so I cannot make a link easily). Thanks. |
The code around f0 is correct. Let me summarize the desired state of affairs so that we have a clear picture of the whole thing:
To be clear what I wrote above is the desired state of affairs as I understand it from your comments. Please correct me if I'm wrong. I think the idea of passing NAN through into the algorithm introduces too much coupling between the C and Fortran code don't you think? By doing so the interface between the C and Fortran code no longer stops at I guess one could make the argument that NaN is part of the interface, i.e. rhobeg could either be not specified or it could be specified as NaN with the same effect, but I notice that despite code changes to support this, none of the documentation is updated to reflect this. Shouldn't it be? Or would it make more sense to just rely on On a separate but related note I've noticed that if I remove the nan checks for rhobeg/rholoc then |
I am still on cellphone so will be brief. You last paragraph is right. I will correct it. I am afraid the first paragraph is wrong: it is possible that f0 is really evaluated to NaN. f0 is different from rhobeg etc. It is not a number freely set by the user. It may be produced by f. I Hope you read my comments on f0 and nlconstr0 again in cobyla.f90. |
See https://github.com/libprima/prima/blob/main/fortran/common/preproc.f90#L17-L30 |
I see what you mean, NaN can be an actually valid value. But I looked at your comment in coblya.f90 and I think this is a good example of why it's not a good idea to accept either "not present" or nan as equivalent. From the user's perspective, if a user provides f0 and it happens to be NaN, but does not provide nlconstr0, then their objective function will be run again needlessly, and the rationale will not be communicated to them. Or if it is communicated to them via a message, it would require them to make a code change to either not pre-compute f0 or to additionally compute nlconstr0. This doesn't seem very user friendly. I read through your comment in preproc.f90#L17-L30. Why are we putting this in Fortran? We can handle these issues within C (and when I say C I'm including the As for |
This is not allowed. The two are either both present or both absent --- don't ask why, or this is the answer:
In summary, if what you said happens, then the Python/MATLAB/Julia/R interfaces are coded wrongly. If some smart user discovers that he/she can provide f0, c(x0), and provides only one of them, the Fortran code will just brutally tell him/her that it is not allowed. |
This is what I believed. I compromised and changed the Fortran code because of our discussion. |
Since f0 and nlconstr0 must be both present or absent, we can tell exactly what is happening by checking whether nlconstr0 is present. Note that the only solver that accepts [f0, nlconstr0] is cobyla. If they are provided to any other solver, a warning should be raised and they will be ignored. Or should we return with an error code? What do you think? |
I agree. Then please correct the code regarding f0 and nlconstr0 while keeping the other parts (rhobeg etc) unchanged. I will merge it, correct the Fortran code regarding |
…as "this argument takes the default value"; warnings will be raised when such values are received; see #135 (comment)
Hi, @nbelakovski , I recall now that
See Therefore, I suggest handling BTW, only NEWUOA, BOBYQA, and LINCOA accept |
As I was working on the Python bindings, I ran into some issues related to memory management. These changes should help with that, particularly the change to the allocation of nlconstr0, since that was the one that appeared to be giving issues with Python. In general I'd prefer to do as little memory allocation within the C interface as possible, since it's challenging enough to have memory management in 1 language, let alone 4 when we start interacting with Python->C++->C->Fortran. What enables the removal of these allocations is that I've discovered that if a variable in Fortran is marked as a pointer and set to null(), then the Fortran function `present` returns false on that variable. This means we can use NULL from within C to indicate that a variable is not provided. This approach has a small inconvenience in that the user will have to handle allocations for optional variables. For example, with f0, the user would have to do the following: double f0 = obj_fun(x0); problem->f0 = &f0; Likewise for things like rhobeg, rhoend, the user would have to do something like the following: double rhobeg = 1.0; options->rhobeg = &rhobeg; double rhoend = 1.0e-6; options->rhoend = &rhoend; This is in constrast to the current way of doing things: options->rhobeg = 1.0; options->rhoend = 1.0e-6; As annoying as the previous option is, it might be preferable since it means we can stop duplicating initialization logic within C. That logic is currently implemented in `prima_init_options` and `prima_check_problem`, but it doesn't replicate the full logic for rhobeg/rhoend. I think that ideally C should be a 'dumb' pass-through and should let Fortran handle as much as possible. We can use this PR to add this functionality to the rest of the variables, but I figured I would start with removing the memory allocations before going too much further.
An intermediate variable was introduced so that the interface will still work if RP=4. The method of providing f0 was changed to be a little bit more user friendly, and rhobeg and rhoend adopted this approach. Additionally, the "unset" value of maxfun and npt was changed to 0 based on discussion in the PR.
9d445ed
to
07ad335
Compare
f4c42ed
to
e9165e4
Compare
@zaikunzhang I've pushed the changes related to maxfun and npt, should be good to go. |
I agree with the logic of the code now. However, does the following code assume that the status of an uninitialized allocatable is unallocated? Does the standard specify this? I guess the status is "undefined", and compilers will complain about it.
|
maxfun and npt were configured so that the default value of 0 is interpeted as not present. The notes on the default values in C were moved from the README to the implementation for better discoverability.
e9165e4
to
7b3a9d2
Compare
It looks like the Windows failures are because Chocolatey failed to download Edit: Looks like it was just a one off issue with chocolatey, things seem fine now. |
As I was working on the Python bindings, I ran into some issues related to memory management. These changes should help with that, particularly the change to the allocation of nlconstr0, since that was the one that appeared to be giving issues with Python. In general I'd prefer to do as little memory allocation within the C interface as possible, since it's challenging enough to have memory management in 1 language, let alone 4 when we start interacting with Python->C++->C->Fortran.
What enables the removal of these allocations is that I've discovered that if a variable in Fortran is marked as a pointer and set to null(), then the Fortran function
present
returns false on that variable. This means we can use NULL from within C to indicate that a variable is not provided. This approach has a small inconvenience in that the user will have to handle allocations for optional variables.For example, with f0, the user would have to do the following:
Likewise for things like rhobeg, rhoend, the user would have to do something like the following:
This is in contrast to the current way of doing things:
As annoying as the previous option is, it might be preferable since it means we can stop duplicating initialization logic within C. That logic is currently implemented in
prima_init_options
andprima_check_problem
, but it doesn't replicate the full logic for rhobeg/rhoend. I think that ideally C should be a 'dumb' pass-through and should let Fortran handle as much as possible.We can use this PR to add this functionality to the rest of the variables, but I figured I would start with removing the memory allocations before going too much further.