clean up github PR form template

[Sevans711]

Closes #1595

Overview

Fixes/improves a variety of outdated/confusing aspects of the default PR form, as noted in #1595:

1. Comment at the top now properly states how to link multiple issues being closed (cannot just do "closes #XXX, #YYY" as previously implied; must repeat the keyword each time, such as "closes #XXX, closes #YYY").
2. Comment at the top now clarifies it is not necessary for PRs to fully close an issue, can instead write something like "Related to #XXX", or "Fixes part of #YYY but does not fully close it."
3. In "Expected Usage" comment, changed "If you are adding a feature into the Internal API, please produce a short example of it in action." to "If this PR adds a new feature, please provide a short example of it in action."
4. Clarified "(if you have label edit permissions)" on the "Added appropriate labels" checklist item
5. Removed reference to nonexistent "docs/interal_api/index.rst"; the checklist item now reads: "Internal (private) function names start with an underscore (_)"
6. Replaced reference to nonexistent "docs/user_api/index.rst"; the checklist item now reads: "User (public) functions have been added to docs/api.rst"
7. Clarified that new example notebooks are not limited to "docs/examples/", they should instead be added to one of the following, depending on example type: "gallery: docs/examples/; guide: docs/user-guide/; quickstart: docs/getting-started/". Also clarified that adding entries to "gallery.yml" only applies for gallery-type examples.
8. Replaced reference to nonexistent "examples.rst". The relevant checklist item now reads: "New notebook examples referenced in appropriate .rst file (gallery: docs/gallery.rst; guide: docs/userguide.rst; quickstart: docs/quickstart.rst)"
9. Rephrased final comment (in "PR Etiquette Reminders") to avoid implying PR submitter needs to request reviewers.
10. Fixed minor typos (e.g., "An issue is linked created and linked")

PR Checklist

General

• An issue is linked created and linked
• Add appropriate labels
• Filled out Overview and Expected Usage (if applicable) sections

[Sevans711]

Should we take this opportunity to add something to the "Testing" part of the PR checklist like "If this PR should improve memory usage or runtime efficiency, at least one benchmark demonstrates expected improvements (add a new benchmark if necessary)"?

[dylannelson]

Interesting and helpful finds. Some I didn't even notice when reading through before this, ha
One thing I'm curious about, the note
- [ ] Added appropriate labels (if you have label edit permissions)
does the "if" part mean that some users can't add labels (including existing ones) or they can't make new labels from scratch?
If it's the first, why might some users not be able to add labels at all (I imagine this isn't the case). If it's the second, is this simply a reference to the case where they may not be able to add an "appropriate" label but also can't make one of their own?

[Sevans711]

Thank you for taking a look!

One thing I'm curious about, the note - [ ] Added appropriate labels (if you have label edit permissions) does the "if" part mean that some users can't add labels (including existing ones) or they can't make new labels from scratch?

It is the former! I believe you need to be granted some nonzero level of permissions for directly editing the uxarray repository in order to actually assign labels to your PR. But, you can certainly open a PR from a fork, even without any permissions at all. For my first uxarray PR I didn't have permissions and opened it from a fork, and I remember being a bit confused about whether I should check this box, and whether I was supposed to be able to add labels somehow but just couldn't figure it out, when in fact you need some edit permissions for it.

Maybe we can workshop the phrasing a bit to come up with something that clarifies this, while also not being too vague? Perhaps something like "Added appropriate labels (if you have uxarray edit permissions)" would be clearer?

[dylannelson]

Oh that's interesting, I would have been confused too. Since some of the other checks have much more added context now, it may help to also add this context too (while also covering the case where they want to check the box but couldn't add labels):

- [] Added appropriate labels, or were unable to add labels (users can't add labels by default)

A bit wordy but may cover all the points of confusion? I avoided the word "edit" because that sounds like a higher level of permission that "using"/"adding", but it seems like the user can either do all or none so it may not matter much either way.

[erogluorhan]

Looks great overall, just a few inline comments

[erogluorhan]

Maybe add something about this section can/should be removed if no work on testing suite was conducted in this PR?

[erogluorhan]

Not only new actually, Any notebooks that the PR's owner played with in their local might need this. Please rephrase accordingly

[erogluorhan]

Oh that's interesting, I would have been confused too. Since some of the other checks have much more added context now, it may help to also add this context too (while also covering the case where they want to check the box but couldn't add labels):

- [] Added appropriate labels, or were unable to add labels (users can't add labels by default)

A bit wordy but may cover all the points of confusion? I avoided the word "edit" because that sounds like a higher level of permission that "using"/"adding", but it seems like the user can either do all or none so it may not matter much either way.

How about "Added appropriate labels (if your repo permissions allow to do so)" @Sevans711

[Sevans711]

Thank you for your feedback and suggestions, @dylannelson and @erogluorhan. I have made changes accordingly; see 6580446:

• Changed phrasing to "Added appropriate labels (if your uxarray repo permissions allow it)".
• Deleted "If an entire section doesn't apply to this PR, comment it out or delete it" comment, to instead provide specific comments in each section. E.g., testing section starts with <!-- If this PR does not update any functionality or tests, remove this section (Testing) -->
• Changed phrasing to "All notebook examples cleared the output…"

Highlighting some lingering questions:

1. Should we take this opportunity to add something to the "Testing" part of the PR checklist like "- [ ] If this PR should improve memory usage or runtime efficiency, at least one benchmark demonstrates expected improvements (add a new benchmark if necessary)"? Also realizing now that I should have pinged @cmdupuis3 on this earlier.
2. Noticing that this bullet point in testing is extremely rigorous: - [ ] Tests cover all possible logical paths in your function and is followed by something far less rigorous: - [ ] Tests are not too basic (such as simply calling a function and nothing else). Does it actually makes sense to keep the extremely rigorous bar of "all possible logical paths"?

• (2a) If no, maybe we can drop it, or rephrase to something like "all major paths" instead?
• (2b) If we're keeping both bullets, I might move the "not too basic" bullet to be before the "all paths" bullet, if that sounds okay?

[cmdupuis3]

1. Should we take this opportunity to add something to the "Testing" part of the PR checklist like "- [ ] If this PR should improve memory usage or runtime efficiency, at least one benchmark demonstrates expected improvements (add a new benchmark if necessary)"? Also realizing now that I should have pinged @cmdupuis3 on this earlier.

Yeah, that would be helpful especially on my draft PRs.

[erogluorhan]

I could even approve it now, but a few quick comments below:

[erogluorhan]

This helps. I don't know if we should be further clear, leaving it up to you regarding something like this: "If this PR does not require any work examples (e.g. new functionality, changes to docs, etc.), remove this section (Examples)"

[Sevans711]

I have a preference for the current phrasing, mostly because I am a bit concerned that implying these types of changes require adding examples is too high a bar to require of all contributors. E.g., including "new functionality" as a reason makes it feel a little like we're effectively saying "if you added a new function, make sure you also updated a notebook or added a new notebook to show how it is used!"

It's hard for me to think of general sorts of changes which actually require adding example notebooks.

The other concern might be that functionality changes could affect existing example notebooks. But, I feel like the burden of checking that existing notebooks still look okay should probably fall onto uxarray's testing suite, e.g. raise an error or warning if a notebook fails to run properly?

[erogluorhan]

That makes sense!

[Sevans711]

I made a slight update, from "If this PR does not update any examples,…" to "If this PR does not affect any example notebooks,…". This way it is a bit broader, but still without implying too high of a bar for contributors not working on examples directly!

[erogluorhan]

This (also related things on Lines 41&42) is okay, but not 100% sure to have them here still:

• Benchmarks do not apply to majority of PRs, so should we bother all contributions with this?
• When they apply, this should somehow be addressed already in practice:
  • the PR owner already knows most of the time (via PR label, milestone, etc.) that they need to look into benchmarks
  • the reviewer knows to look into benchmarks for such PRs

[Sevans711]

Thank you for your feedback, I understand what you're saying and I agree many contributions might not need this. But, for contributions where it is relevant, it may serve as a nice reminder.

I was motivated to look into it a bit further in order to reply, and I think I found a few good recent examples where benchmarks maybe should have been run, but were not:

• Move imports for should-be-optional dependencies #1606 should cause speedup to import uxarray because it makes more imports lazy. I'm pretty sure it did cause a speedup, and if there was a reminder I would have run benchmarks, but there was no reminder and nobody ran benchmarks there.
• Optimized connectivity: Simultaneous face_edges and edge_nodes #1560 claims to optimize something / improve performance, and multiple people with permissions to apply the "run-benchmark" label have looked at this PR, but nobody has added that label yet in order to actually check what the benchmarks say. (Not trying to blame anyone here, just trying to say this supports the argument for adding a checklist item as a reminder.)
• EFT-based compensated arithmetic for spherical geometry (GCA intersection, point-in-face, face bounds) #1513 makes performance claims from the very start, and a variety of timing tests were run during the PR discussion, but the full uxarray ASV benchmarking suite was not run, from what I can tell. I suppose this case is less about benchmarks demonstrating improvements, though, and more about benchmarks not demonstrating "too much" slowdown.

Maybe the proposed checklist item should be rephrased to something more neutral like this? "If expecting performance efficiency changes, at least one ASV benchmark shows changes (add new benchmarks if necessary)."

[erogluorhan]

This sounds good!

[Sevans711]

I turned this phrasing over for a while because PR 1513 didn't actually expect performance changes; comments instead claimed roughly-the-same performance for the most part. I also added a comment reminding how to run benchmarks, and clarifying that contributors without any uxarray edit permissions don't need to worry about checking this box initially.

Here's where it ended up:

- [ ] If this PR could affect performance, ran ASV benchmarks and confirmed they show expected behavior (add a new benchmark if necessary)
<!-- Adding the run-benchmark label (if your uxarray repo permissions allow it) will run ASV benchmarks.
    If you need benchmarks to be run but don't have permissions, leave this item unchecked for now. -->

I changed the comment at the top of the section as well to clarify it's not just efficiency improvements, but also any likelihood of affecting performance, would be enough to prevent removing this section.

[Sevans711]

@erogluorhan Thank you again for reviewing! Pinging to let you know I made some minor changes (see replies to inline comments above, as well as small notes below). If you still approve there is no need to follow up here, just wanted to keep you in the loop.

Extra changes not included as replies above:

• moved "Tests are not too basic" checklist item to be above "Tests cover all paths" item.
• as per Slack conversation and my comment earlier in thread here, changed wording from "Tests cover all possible logical paths" to "Tests cover all major paths".

[dylannelson]

Read through it all again and think it's overall much more clear, covers additional meaningful reminders, and sounds like it could be understood by most users.