Skip to content

Avoid copying non-contiguous data in CompImageHDU - #14425

Merged
saimn merged 2 commits into
astropy:mainfrom
astrofrog:non-contiguous-compressed
Feb 21, 2023
Merged

Avoid copying non-contiguous data in CompImageHDU#14425
saimn merged 2 commits into
astropy:mainfrom
astrofrog:non-contiguous-compressed

Conversation

@astrofrog

@astrofrog astrofrog commented Feb 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

The fix from #9958 is no longer required as with the new tiled compression module we convert each tile to bytes using tobytes() which effectively returns contiguous bytes, before we pass the bytes to the various compression algorithms. I have removed the overall conversion of the data array to be C contiguous, and have adjusted the test to really check that it works with a couple of dtypes (with/without quantization which does a copy in itself) and for all the supported compression types.

When passing non-contiguous arrays to CompImageHDU, this should make things a bit more efficient memory-wise and also time wise, but also importantly will ensure that CompImageHDU.data is not changed in-place.

I don't think this needs a changelog entry - but this did make me wonder where performance improvements should go in the current changelog system - other entries can't be assigned to specific sub-packages, but maybe we should allow this?

This addresses the second bullet point in #3895

@astrofrog
astrofrog requested a review from saimn as a code owner February 21, 2023 12:08
@github-actions

github-actions Bot commented Feb 21, 2023

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@pllim pllim added this to the v5.3 milestone Feb 21, 2023
@pllim

pllim commented Feb 21, 2023

Copy link
Copy Markdown
Member

If this fixes some long-standing performance issue, should add a change log. Thanks!

@astrofrog

astrofrog commented Feb 21, 2023

Copy link
Copy Markdown
Member Author

I'm not sure that anyone has ever complained about the performance issue as such hence why I don't think it matters. But we should definitely remove this code as it is unnecessary.

@saimn

saimn commented Feb 21, 2023

Copy link
Copy Markdown
Contributor

I think we usually use "feature" for perf improvements and the idea was to avoid using "other" too easily. But maybe we can rethink a bit the changelog sections, maybe add a few ones (e.g. deprecation warnings could be more visible?).

Anyway for this case probably no need for a changelog entry, it's good to avoid a copy but probably not a huge performance improvement.

saimn
saimn approved these changes Feb 21, 2023
View reviewed changes

@saimn saimn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice simplification, thanks @astrofrog !

@saimn
saimn merged commit 67faffb into astropy:main Feb 21, 2023
@saimn saimn mentioned this pull request Feb 21, 2023
Open
3 of 4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saimn saimn saimn approved these changes

Assignees

No one assigned

Labels

io.fits no-changelog-entry-needed Performance

Projects

None yet

Milestone

v5.3

Development

Successfully merging this pull request may close these issues.

3 participants

@astrofrog @pllim @saimn