-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Optimize fits.Header parsing #8428
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8428 +/- ##
==========================================
+ Coverage 86.77% 86.78% +<.01%
==========================================
Files 387 387
Lines 58109 58125 +16
Branches 1060 1060
==========================================
+ Hits 50426 50442 +16
Misses 7068 7068
Partials 615 615
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few question, but on the whole this seems like a great addition (given the timings).
Did you run a profiler against the benchmark? If so I'm very interested to see where the time is spent here. But I can also do that later (probably after the weekend).
astropy/io/fits/card.py
Outdated
@@ -24,6 +24,7 @@ | |||
KEYWORD_LENGTH = 8 # The max length for FITS-standard keywords | |||
|
|||
VALUE_INDICATOR = '= ' # The standard FITS value indicator | |||
VALUE_INDICATOR_LEN = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be more semantically correct to replace the 2
with len(VALUE_INDICATOR)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I hard coded the value to avoid a tiny overhead at import time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overhead is probably negligible, I can change if you prefer. But the value is hardcoded and defined just above so using len
seems overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe overkill, but I only commented because it took me a few seconds to figure out what this value is (probably more time than saved by hardcoding it 😄).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for changing it 👍
astropy/io/fits/header.py
Outdated
@@ -93,7 +93,9 @@ def __init__(self, cards=[], copy=False): | |||
|
|||
.. versionadded:: 1.3 | |||
""" | |||
self.clear() | |||
self._cards = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, why this change? Isn't this just duplicating code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a good change. It's identical to clear
and the method clear
seems much more obvious to me (even without looking at the method) than these 3 re-assignments.
I'd prefer the previous call self.clear()
here.
header = cls() | ||
for idx, card in enumerate(cards): | ||
header._cards.append(card) | ||
keyword = Card.normalize_keyword(card.keyword) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be copied from the append
method, couldn't this be factored into a separate method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can find a good name yes ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_add_card
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method could not contain the _cards.append
since .append
can also insert the card at any position ... so it would just take care of the indices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate? I don't understand the previous comment.
@@ -938,13 +953,14 @@ def keys(self): | |||
instance has the same behavior. | |||
""" | |||
|
|||
return self.__iter__() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this changed? It seems unlikely this will affect performance, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About this and .clear
above, I included a commit to change a few non-obvious lines of code:
04f2c36
I think that these changes help a lot in the understanding of the flow in this complex class. For instance in __init__
I prefer to duplicate three lines instead of having to search each time where some variable is defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it makes it harder to understand the semantics. Searching for definitions is an IDE
thing while understanding the semantics and invariants is a Developer thing. And fits is complicated enough we shouldn't make it harder.
This is just me thinking. If that makes it easier for you to maintain the module go ahead (you're a lot more active than me so it's your judgement call). :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the change makes thing harder ;), if you look at the items
, keys
and values
definition it is also much more consistent and easier to understand now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay sorry, maybe this was the worst comment for my argument. 😅
I don't mind the changes to keys
and values
(much).
CAn you share the fits or the code to produce one that's like it? |
Thanks for the profiling! May seem like a stupid idea but could you put the non-performance changes into a different PR, because I really like the performance improvements and I don't want the PR to suffer from my nitpicking comments about the non-performance-related changes? Or is that too much trouble? |
Ok, I reverted the change for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! The only thing I'm not too fond of is the introduction of VALUE_INDICATOR_LEN
- it is a very small speed improvement for a bit of lack of readability. But not a big deal at all.
CircleCI failure is #8431 and unrelated. Since there are 2 approvals, I am merging this. Thanks! |
By the way, do we need a benchmark for this over at https://github.com/astropy/astropy-benchmarks ? |
Related to #5593, this implements one of the idea mentioned there: adding a
fromcards
class method to speed up the creation of a Header instance from a list of cards.The other main change is some reorganization of keyword parsing (
_parse_keyword
), improving the most common case.And probably a small bugfix with the return value from
_check_if_rvkc_image
which was not used. This was probably harmless, I guess the check was done again later.The gain is significant (~12-13%) for files with many HDUs, below just computing the number of HDUs for a file with >300 extensions (the second command is using 100 extensions, the third only 10).
Before:
After:
I'm also working on other ideas from #5593, but this will take more time, so let's go with this for now!