Repository navigation
Fix up lazy loading issues - #5589
Conversation
| # max_idx. Note we do not try to be clever about skipping HDUs | ||
| # even though key.step might conceivably allow it. | ||
| for i in range(number_loaded, max_idx): | ||
| print(i) |
| # have been loaded before passing the slice on to super. | ||
| if isinstance(key, slice): | ||
| max_idx = self._positive_index_of(key.stop) | ||
| if max_idx == sys.maxsize: |
There was a problem hiding this comment.
I'm wondering about this if, did this make trouble in some tests?
There was a problem hiding this comment.
No, I was just trying to catch the case when the slice had no upper bound specified. I think I can roll it in to the if below, though.
There was a problem hiding this comment.
Just kidding. Turns out sys.maxsize is the value in python 2.7. In python 3 it is None, so special handling of this case is necessary.
|
And re: #5582, FYI : that's definitely for 2.0 as it would require at least one version of deprecation before changing the behavior. |
|
I'm not quite sure what you mean by "iteration not working". In the basic case iteration was working, just not iteration over a slice. |
| try: | ||
| self._try_while_unread_hdus(self.index_of, item) | ||
| except KeyError: | ||
| except (KeyError, TypeError): |
There was a problem hiding this comment.
I think it would be fine, instead of changing this, to change index_of back to just raising a KeyError if the type of key is invalid. I'm not entirely sure what motivated me to change that in the first place.
Thanks for fixing this!
|
LGTM, thanks @mwcraig |
|
This PR fixes the problem in #5585. I am trying to understand a different regression problem we see caused by the lazy loading PR and not fixed by this one. |
|
The other problem we are seeing is due to |
| if not isinstance(_key, string_types): | ||
| raise TypeError( | ||
| raise KeyError( | ||
| '%s indices must be integers, extension names as strings, ' |
There was a problem hiding this comment.
Minor nitpicking, but could you change these strings here and a few other places in the file to use the .format() formatting instead of %
|
@astrofrog -- will do later today. |
eteq
added
Affects-dev
|
backported to v1.3.x in 459a15c |
Fix or address these issues which resulted from lazy loading. In each case the goal is to maintain the current (1.x) behavior of io.fits.
inoperator (closes ccdproc failures infits.HDUList.index_of#5583)[ ] Address behavior ofWon't change unless someone requests it.closeAllow HDUs to not be loaded at close() to gain full benefit of lazy loading #5582@eteq -- I believe I've milestoned this correctly, but please modify if not.
edit: changed text of second todo.