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

Add sky map pad method #624

Closed
wants to merge 1 commit into from
Closed

Add sky map pad method #624

wants to merge 1 commit into from

Conversation

cdeil
Copy link
Contributor

@cdeil cdeil commented Jul 8, 2016

This pull request adds a pad method to SkyMap, to help implement SkyMap.downsample in #615 .

Coded with @OlgaVorokh

@cdeil cdeil added the feature label Jul 8, 2016
@cdeil cdeil added this to the 0.5 milestone Jul 8, 2016
@cdeil cdeil self-assigned this Jul 8, 2016
@@ -387,6 +387,48 @@ def cutout(self, position, size):
skymap = SkyMap(data=cutout.data, wcs=cutout.wcs, unit=self.unit)
return skymap

def pad(self, factor, mode, **kwargs):
Copy link
Member

@adonath adonath Jul 8, 2016

Choose a reason for hiding this comment

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

I think for a general public padding method this factor argument might be too special. It is hard to understand from the docs how it is actually used. Looking at the code I presume it something like:

Pad image to the nearest larger shape, that is divisible by the given factor in both axis.

For a general purpose padding function I find a pad_width argument much more intuitive.

@cdeil cdeil mentioned this pull request Jul 18, 2016
@cdeil
Copy link
Contributor Author

cdeil commented Jul 18, 2016

@OlgaVorokh will continue this PR in #630. Closing this now.

@cdeil cdeil closed this Jul 18, 2016
(16, 12)
"""
pad_width = factor - (np.array(self.data.shape) % factor)
pad_width = [(0, pad_width[0]), (0, pad_width[1])]
Copy link
Member

@adonath adonath Jul 18, 2016

Choose a reason for hiding this comment

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

Thinking a bit more about this, I still prefer symmetrical padding (i.e. on all edges), so that the center of the image is unchanged (or at least add this as an option). The down- and upsampling can create artefacts at the data edges, depending on the boundary handling, that is chosen. I would find it weird, if this only affected the right and upper edge of the image. I would rather expect a consistent treatment of all the edges, e.g. how it's done for convolution operations.

And typically users will have a source in the center of the map. If the map is padded (for whatever reasons...), this will move the source out of the center of the map. Which is not that nice e.g. when it's plotted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants