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

Override HDUList.__add__() to return an HDUList instance #7284

Closed
wants to merge 1 commit into from

Conversation

@ritiek
Copy link
Contributor

commented Mar 10, 2018

As mentioned in #7185 (comment), adding two slices (or HDUList objects) returns a list. We can fix this behavior and return an HDUList by overriding HDUList.__add__().

@astropy-bot

This comment has been minimized.

Copy link

commented Mar 10, 2018

Hi there @ritiek 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

I noticed the following issue with this pull request:

  • Changelog entry section (v3.1) inconsistent with milestone (v3.0.2)

Would it be possible to fix this? Thanks!

If there are any issues with this message, please report them here.

@pllim pllim added this to the v3.0.1 milestone Mar 10, 2018
@pllim pllim requested review from drdavella and saimn Mar 10, 2018
@nden

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2018

Ignoring the fact that I don't understand why it is needed to support this functionality in the library, the implementation allows an HDUList object to be extended with an arbitrary list. It can be extended with another fits file in which case the result has two PrimaryHDUs, or really any other list of objects (int, Quantity, cats, ...). If there is a compeling reason to add this feature we should be more careful how we add it.

@bsipocz bsipocz modified the milestones: v3.0.1, v3.0.2 Mar 12, 2018
@saimn

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2018

I agree with @nden about the need to check for duplicates PrimaryHDUs. Currently .append checks this, but not extend as it is not overloaded from list.
So I'm 👎 on this, given the other discussions about list methods for HDUList, I think we should limit the number of list methods and maybe even remove the list inheritance.

@ritiek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2018

Yep, I am in favor of removing list inheritance as well. It will probably make things easier to understand. I'll close this PR.

@ritiek ritiek closed this Mar 16, 2018
@ritiek ritiek deleted the ritiek:add-hdulists branch Apr 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.