-
Notifications
You must be signed in to change notification settings - Fork 11
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
Issue#75 sa triangle #527
base: devel
Are you sure you want to change the base?
Issue#75 sa triangle #527
Conversation
lasofivec
commented
Jun 30, 2021
Hello @lasofivec! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-12-21 22:01:02 UTC |
Shall we wait for this PR before the deployment of 1.4.11? |
Hum... the tests for the 1.4.11 are still failing because of openadas, so it looks like you have more time |
npoly, | ||
ltri, | ||
num_threads | ||
) |
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.
@lasofivec, here the routine _vt.triangulate_polys()
is called with the first argument as &data[0]
Here data is defined as data = <double **> malloc(npoly * sizeof(double *))
But in line 2812 (in routine def vignetting()
, the same routine is called with the first argument as data
.
And there data
is defined as data = <double **> malloc(nvign*sizeof(double *))
So in both cases, data
refers a similar object (a double pointer to an array of type double
?).
But in here it is provided as a memory address of the first element (&data[0]
), while in line 2812 it is provided directly (data
)
Questions:
- Why this difference ?
- Is it a bug ?
- Which one is correct ?
The definition of the routine, in _vignetting_tools.pyx
(line 291) says it should have a double pointer of type double**
as first argument. Which leads to think that only the first case (here, line 5170 is correct).
More questions:
- Why provide
&data[0]
and not&data[ii]
? - Why use a double pointer instead of a single pointer in the routine definition ?
- Why provide a pointer only to the first element to
data
?
# normally it should be a sum, but it is a minus cause is we have (U,-V) | ||
sumc = ucrossv[ii] | ||
return sumc >= 0. | ||
_bgt.compute_cross_prod(u, v, &ucrossv[0]) |
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.
why give a pointer (&ucrossv[0]
) ? The routine takes an array (see definition)
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.
To be sure it will take the actual array and not a copy of it. So that the array is modified on exit. Not really necessary but some people prefer to do it to make sure everything is modified properly.
And an array is a pointer.