Skip to content

Implement pixel axis sharing in CompoundWCS - #98

Merged
astrofrog merged 2 commits into
astrofrog:low-level-wcs-wrappersfrom
Cadair:low-level-wcs-wrappers-shared-compound
Apr 24, 2020
Merged

astrofrog merged 2 commits into
astrofrog:low-level-wcs-wrappersfrom
Cadair:low-level-wcs-wrappers-shared-compound

Conversation

@Cadair

@Cadair Cadair commented Apr 22, 2020

Copy link
Copy Markdown
Collaborator

Defo needs more tests, but mainly we need to decide what the correct behaviour of world_to_pixel_values is.

@astrofrog astrofrog left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few comments below

for i, ix in enumerate(self.mapping.mapping):
if out_shape[ix] != pixel_shape[i]:
raise ValueError(
"The pixel shapes of the supplied WCSes do not match for the dimensions shared by the supplied mapping.")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we validate this in the input to avoid doing this check every time this property is accessed? (same comment for pixel_bounds)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair point, but that's basically doing to duplicate this whole property for what I can't imagine is going to be a big gain?

The WCSes to combine
"""
def __init__(self, *wcs):
def __init__(self, *wcs, mapping=None):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be included in docstring above

return tuple(world_arrays)

def world_to_pixel_values(self, *world_arrays):
# TODO: Decide on the behaviour here!

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤯

@Cadair
Cadair force-pushed the low-level-wcs-wrappers-shared-compound branch from f4739fa to ecf00e3 Compare April 24, 2020 13:52

@astrofrog astrofrog left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@astrofrog
astrofrog merged commit 5a5737e into astrofrog:low-level-wcs-wrappers Apr 24, 2020
@Cadair
Cadair deleted the low-level-wcs-wrappers-shared-compound branch April 24, 2020 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants