Added a 'perf' changelog entry type#16694
Conversation
|
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.
|
neutrinoceros
left a comment
There was a problem hiding this comment.
pre-commit will still invalidate a fragment named, for instance 1.perf.rst, but the fix is simple enough:
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 64cbacf354..e663730c60 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -114,10 +114,10 @@ repos:
name: changelog filenames
language: fail
entry: >-
- changelog files must be named <sub-package>/####.(bugfix|feature|api).rst
+ changelog files must be named <sub-package>/####.(bugfix|feature|api|perf).rst
or ####.other.rst (in the root directory only)
exclude: >-
- ^docs/changes/[\w\.]+/(\d+\.(bugfix|feature|api)(\.\d)?.rst|.gitkeep)
+ ^docs/changes/[\w\.]+/(\d+\.(bugfix|feature|api|perf)(\.\d)?.rst|.gitkeep)
files: ^docs/changes/[\w\.]+/
- id: changelogs-rst-other
name: changelog filenames for other category|
@neutrinoceros - fixed, thanks! |
|
I think there isn't any merged PRs that need their change log moved. But should we add a note to these ones? |
|
Announcing performance improvements in the change log can be good, but we should write down a guideline somewhere that in order for a pull request to be announced in the change log in the performance category there must exist some code example using only the public API that has been sped up by e.g. 10% (the number is to be decided here). |
|
I would say even 2-3% improvements. These add up over time! But maybe others feel 5% is a better number to merit the changelog? |
|
@eerovaher , are you saying that this PR also needs to update dev docs? |
|
If a performance improvement isn't important enough to be in the changelog maybe it doesn't warrant a pull request? 🙃 |
|
I think at least this needs updating? https://docs.astropy.org/en/latest/development/development_details.html#add-a-changelog-entry |
The smaller the speedup, the more difficult it is to distinguish the signal from the noise. If the speedup is too small then measuring it reliably is a waste of time and effort.
All the other change log entry categories are documented, there's no reason for the performance category to be any different.
Some refactoring pull requests might have small performance implications. In some cases these serendipitous speedups might be large enough to be announced. |
|
Maybe the threshold should just be a 'measurable' performance improvement. |
…dd note about ensuring the change is measurable using public API
|
I've added a note to the contributor docs, and also have added in both cases that the improvement has to be measurable using public API, which I think is a sufficient threshold. |
pllim
left a comment
There was a problem hiding this comment.
OK let's do it then. Thanks, all!
* Added a 'perf' changelog entry type * Adjust pre-commit configuration * Add mention of 'perf' changelog entry type in contributor docs, and add note about ensuring the change is measurable using public API
As discussed in #16679
Fix #16679