Added two optional arguments to Header.remove and removed AstropyDeprecationWarning

[Pratik151]

addresses #4429
Does this look good?
I am looking into docs now and will change it soon.

[PratikPatel13]

https://github.com/astropy/astropy/blob/master/astropy/io/fits/tests/test_header.py#L1386
Will add the test for Header.remove if this is good.

[embray]

Will add the test for Header.remove if this is good.

This is why it's good to write tests first, and run the tests before committing. It'll catch things like syntax errors, of course, but will also make sure sure (assuming the test itself is written well) that your code does the right thing, etc.

[embray]

Ah, I see now that the original code went through self.index too, so that's probably why you kept it that way. :) Yeah, it would be a slight enhancement for this to go directly through self._keyword_indices instead.

Though it occurs to me now that since the keyword variable here is user-input, it should also be "normalized" before being compared with the entries in self._keyword_indices (this already happens in self.index which may be why I used it originally).

So before doing anything in this method run:

keyword = Card.normalize_keyword(keyword)

Users are sometimes inconsistent about how they write FITS keywords--so this puts them in a "normalized" form where they're all upper-case, etc.

[embray]

Nitpicks aside, this is the right idea and on a good track. Thanks @Pratik151

[embray]

Oh, and another thing I meant to point out....

Now that this is removed, the _is_astropy_internal function isn't used anywhere, so you can just remove this function entirely too (as well as the imports of inspect and sys since they aren't used anywhere in this module once _is_astropy_internal is removed).

[Pratik151]

@embray Will update all things and also add tests.
Thank you for explaining clearly :)

[Pratik151]

@embray Sorry for being late, I started to work on this today.

When I try to delete keyword which is not present using
>>> del h1[h1._keyword_indices['A'][0]]
It is throwing IndexError and not KeyError and it is also adding the keyword with empty list as value. So for above new keyword 'A' is added with value of empty list in _keyword_indices
Here it is:

>>> h1 = fits.Header([('A','B'),('C','D')])
>>> h1._keyword_indices
defaultdict(<class 'list'>, {'A': [0], 'C': [1]})
>>> del h1[h1._keyword_indices['D'][0]]
Traceback (most recent call last):
File "", line 1, in
IndexError: list index out of range
>>> h1._keyword_indices
defaultdict(<class 'list'>, {'A': [0], 'C': [1], 'D': []})


Should this behave like dictionary and throw KeyError or it is working as intended?
If 'A' is not present
>>>>h1._keyword_indices['A']
[]
adds 'A' to keywords list with value to empty list.

I am able to work with this PR with some condition checking but just wanted to make sure behaviour of _keyword_indices is correct or not

[saimn]

@Pratik151 _keyword_indices is a defaultdict, so its behavior is expected: when you access a key that doesn't exist, it creates a default value, a list in this case because _keyword_indices is created with defaultdict(list)

[Pratik151]

@embray Does this look good? or I need to change something?

[embray]

As @saimn wrote self._keyword_indices is a defaultdict. You should still check if the keyword is in the dict before trying to use it. Eg:

if 'A' in self._keyword_indices:
    del self[self._keyword_indices['A'][0]]

Otherwise attempting to access self._keyword_indices['A'] will cause an empty entry for 'A' to be created, and in this case violate the invariant that if a keyword is in self._keyword_indices, the associated list is non-empty. I haven't looked at your code yet though so we'll see what you did...

[embray]

I see now, I think I misled you by forgetting, myself, that keyword_indices is a defaultdict, and won't raise a KeyError.

In this case adding the try/except overcomplicates things. Just change this to:

if keyword in self._keyword_indices:
    del ...
    if all:
        ...etc...
elif not ignore_missing:
    raise KeyError(....)

Make the text for that last KeyError the same as this one. You can see that the end result is the exact same logic, but vastly simplified.

[Pratik151]

@embray Thanks for review. Simplified the code :)

[embray]

Looks good. Now it just needs a changelog entry. Three really. Two for the new arguments under "New Features" and one under "API Changes" for the change in header keyword deletion.

[Pratik151]

@embray Added changes to CHANGES.rst. Should I add more description in API changes?

Travis CI build is failing because of some conflicts in packages.

Hint: the following packages conflict with each other:
- numpy 1.9_
-matplotlib 1.5|1.5.0_

[bsipocz]

The travis docs build failure is unrelated to this PR, the build should pass once restarted.

[Pratik151]

@embray can you restart the build for this and review :) .

Sorry for pinging you again.

[astrofrog]

@Pratik151 - can you rebase this? It looks like there is a conflict

[Pratik151]

@astrofrog I will do it in couple of days. I am out now and don't have access my machine here.

[bsipocz]

@Pratik151 - Could you rebase please?