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

Issue#75 sa triangle #527

Open
wants to merge 38 commits into
base: devel
Choose a base branch
from
Open

Issue#75 sa triangle #527

wants to merge 38 commits into from

Conversation

lasofivec
Copy link
Collaborator

sa_map_poly0_blockFalse_steprz0_01

@pep8speaks
Copy link

pep8speaks commented Jun 30, 2021

Hello @lasofivec! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 178:80: E501 line too long (80 > 79 characters)

Line 388:80: E501 line too long (87 > 79 characters)
Line 443:80: E501 line too long (82 > 79 characters)
Line 471:24: E231 missing whitespace after ','
Line 472:17: E265 block comment should start with '# '
Line 473:41: E231 missing whitespace after ','
Line 539:80: E501 line too long (80 > 79 characters)
Line 550:21: E265 block comment should start with '# '
Line 551:45: E231 missing whitespace after ','

Comment last updated at 2021-12-21 22:01:02 UTC

@Didou09
Copy link
Member

Didou09 commented Jun 30, 2021

Shall we wait for this PR before the deployment of 1.4.11?
It has to be done before 13h today, what do you think?

@Didou09
Copy link
Member

Didou09 commented Jun 30, 2021

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
)
Copy link
Member

@Didou09 Didou09 Jul 20, 2022

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])
Copy link
Member

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)

Copy link
Collaborator Author

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.

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.

3 participants