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

Support arrays for nside #71

Closed
wants to merge 3 commits into from
Closed

Conversation

lpsinger
Copy link
Contributor

Fixes #66.

@astrofrog astrofrog self-requested a review April 16, 2018 22:57
@cdeil
Copy link
Member

cdeil commented Apr 17, 2018

@lpsinger - Thanks!
Changes look good to me.
Adding one very simple concrete test where you pass nside with arrays of different shapes would be useful IMO. But I think the tests where you np.tile nside actually do that, and if so, then OK to leave as-is.

I think these crashes are unrelated; they look like the Cython bug we had earlier this year:
https://travis-ci.org/astropy/astropy-healpix/jobs/367312532#L355
https://travis-ci.org/astropy/astropy-healpix/jobs/367312533#L1047

@bsipocz or @astrofrog - What do we have to do to resolve that? Update astropy-helpers?

@cdeil cdeil added this to the 0.3 milestone Apr 17, 2018
@lpsinger
Copy link
Contributor Author

Side note: it would eliminate the need for a lot of the manual array broadcasting and datatype checks if the cython functions were implemented as ufuncs.

@cdeil
Copy link
Member

cdeil commented Apr 17, 2018

@lpsinger - Can you please make that suggestion about ufuncs in a new issue?
Otherwise there's the risk that this point just gets lost once this is merged.

Probably @astrofrog knows what you mean already, but for me or others, could you please include a link to a Cython example what exactly you propose to change to?

@lpsinger
Copy link
Contributor Author

Probably @astrofrog knows what you mean already, but for me or others, could you please include a link to a Cython example what exactly you propose to change to?

Done. See #72.

@cdeil
Copy link
Member

cdeil commented Apr 18, 2018

The unrelated Cython crashes mentioned above have been fixed via #74 .
Restarting CI builds now.

@cdeil cdeil self-assigned this Apr 18, 2018
@cdeil
Copy link
Member

cdeil commented Apr 18, 2018

@astrofrog
Copy link
Member

I think it's the usual issue that Windows defaults to 32-bit ints

@lpsinger
Copy link
Contributor Author

Yuck. The only way around this that I see is to cast nside to np.int64 in the tests.

I think that if we rewrote these functions as ufuncs (see #72) we would get casting like that automatically. @astrofrog, is this approach OK with you?

@cdeil
Copy link
Member

cdeil commented Apr 24, 2018

The only way around this that I see is to cast nside to np.int64 in the tests.

This is just for tests that call directly into the Cython functions, and there you just have to pass the correct types, no? Users won't see these errors because they go through the Python wrapper functions which do the type casting to np.int64 for them.

@lpsinger - Maybe do this change and merge this in? The discussion whether to use ufuncs instead and re-write the whole package will probably take longer (#72), and having the feature & tests here merged is more helpful than not, no?

One more thought: I see many <int> in your Cython code. Are those 64 bit? Related: #65 by @astrofrog .

@lpsinger
Copy link
Contributor Author

@lpsinger - Maybe do this change and merge this in? The discussion whether to use ufuncs instead and re-write the whole package will probably take longer (#72), and having the feature & tests here merged is more helpful than not, no?

I would like to play around a bit longer with #72 first. I think that it may help with the int type too.

@cdeil
Copy link
Member

cdeil commented Jun 28, 2018

@lpsinger - We'll have the astropy-healpix coding sprint next Monday / Tuesday, and support for nside arrays is very high on the list of things we need. Do you have time to try and sort out the int here so that this can go in? Or do you really think the change to C / ufuncs should be done first before adding this?

@lpsinger
Copy link
Contributor Author

@lpsinger - We'll have the astropy-healpix coding sprint next Monday / Tuesday, and support for nside arrays is very high on the list of things we need. Do you have time to try and sort out the int here so that this can go in?

Next week I will.

Or do you really think the change to C / ufuncs should be done first before adding this?

I think that we should close pull request and just switch to ufuncs because this will kill two birds with one stone.

@cdeil cdeil closed this Jun 28, 2018
@cdeil cdeil mentioned this pull request Jun 28, 2018
@lpsinger lpsinger deleted the array-nside branch March 20, 2021 02:35
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