Closed adl1995 closed 7 years ago
@cdeil - I just made another commit. Hopefully this resolves all the issues.
See https://travis-ci.org/hipspy/hips/jobs/249595428#L1148 (always run tests locally before pushing)
Before we merge I have two questions on user interface, @tboch and @adl1995 - please comment:
create
and create_simple
aren't great. I don't have a suggestion for better names. Thoughts?width
and height
or nx
and ny
for the number of pixels? (In https://github.com/hipspy/hips/issues/39#issuecomment-311889775 @tboch commented that he's taking width and height in Aladin Lite). I think if we take a shape
, especially with the current order (ny, nx)
, but even if we flip, users will get it wrong all the time . So here I'm in favour of either width
and height
or nx
and ny
instead of shape
.@cdeil - How about create_from_fov
? I'm fine with taking two separate arguments instead of shape
.
I made another commit. However, as I pulled the latest code from master
branch, I now have a commit named:
Merge branch 'master' of https://github.com/hipspy/hips into create_wcs
The common way to avoid these merge commits is to "rebase" instead of "merging". Please read the Astropy dev docs (or other resources) explaining this and do that in the future. For this PR: either leave as-is, or fix it up (e.g. by squashing commits, or doing a hard reset on the commit before the merge commit).
@cdeil - I have removed the merge commit.
Things look fine for me.
I like @cdeil 's suggestion to split the shape
argument into width
and height
. Do we leave that for another pull request?
As discussed in telcon: suggest to keep Shape
attribute on WCSGeometry
as-is, but only to take parameters nx
and ny
separately in constructors and make and store the Shape
on self.
Correction: call it width and height (both in the constructor arguments, and in the Shape
attributes.
@cdeil I have made a commit updating the shape
parameter.
@cdeil I have updated the test cases. The build is passing now. However, I did make some changes on my local notebook, but the results looked identical to me. Please verify before merging.
@adl1995 - Thanks for fixing the bug where the values in crpix
were switched in create_simple
.
I've attached two commits here, one with minor cleanup, and one fixing this test fail: https://gist.github.com/cdeil/04226e02ea9c924d24ae91d53e75dcf5
For now I've simply updated the reference value, I'm not sure which is correct. Also, the high-level docs example is broken now (stuck fetching tiles forever).
I think we should just merge the open PRs in today, especially the tile order computation step, and then re-start debugging.
Added a classmethod as mentioned in #39.