Stabilize diffraction module

[tommy-waltmann]

Description

This PR removes the notice saying the diffraction module is unstable, and updates the changelog with a statement saying the API is now stable.

Motivation and Context

This PR should be the last one merged before the 2.9.0 release, which I plan to make in the near future. The actual changes that stabilize the diffraction module's API were completed in #937 , so this PR is here to alert users to the slight API changes before the next release goes out.

How Has This Been Tested?

No tests needed for this PR.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds or improves functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation improvement (updates to user guides, docstrings, or developer docs)

Checklist:

• I have read the CONTRIBUTING document.
• My code follows the code style of this project.
• I have updated the documentation (if relevant).
• I have added tests that cover my changes (if relevant).
• All new and existing tests passed.
• I have updated the credits.
• I have updated the Changelog.

[codecov]

Codecov Report

Merging #940 (7fdddc8) into master (c09e072) will not change coverage.
The diff coverage is n/a.

❗ Current head 7fdddc8 differs from pull request most recent head a73bc68. Consider uploading reports for the commit a73bc68 to get more accurate results

@@           Coverage Diff           @@
##           master     #940   +/-   ##
=======================================
  Coverage   55.22%   55.22%           
=======================================
  Files          15       15           
  Lines        2586     2586           
  Branches       38       38           
=======================================
  Hits         1428     1428           
  Misses       1142     1142           
  Partials       16       16           

Impacted Files	Coverage Δ
freud/diffraction.pyx	62.06% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c09e072...a73bc68. Read the comment docs.

[bdice]

Issue #750 is a known, non-trivial problem with the DiffractionPattern class that hasn't been resolved. We also have three open PRs that affect this module: #761, #793, #936. I would feel better about marking this API stable once all of those are resolved, but I don't think we are ready yet.

[tommy-waltmann]

I will admit that I don't know all the details of #761 (fixing #750 ) or #793 because others were working on them a long time ago, but based on their descriptions I wouldn't think those PRs would introduce API changes to the diffraction module. In addition #936 shouldn't introduce API changes.

When we declare a module stable we are simply declaring that the API won't change without a major version bump, right?

[bdice]

When we declare a module stable we are simply declaring that the API won't change without a major version bump, right?

An API stability promise is the primary part of the declaration, but I also think there is a secondary promise of behavior (up to any bugs that are found). I hesitate to promise that we won't change APIs while knowing that the current behavior has a significant bug. Even if the API remains unchanged, the behavior must change in a significant way if the bug is fixed. Users relying on this code today could see significant changes in later 2.X releases, which doesn't feel "stable."

We don't really gain anything by marking this API as stable, and I think it is helpful to let users know that it is less polished than the rest of freud.

[tommy-waltmann]

Okay, in that case I'll close this and make the release later this week without any stability guarantees for the diffraction module.

[DomFijan]

When I last spoke with @andkerr I was under the impression that #793 and #761 are basically supposed to solve the same thing - or #793 was supposed to just solve a small subset of issues in #750 . I don't know if @andkerr has plans to finish #793 or not but since he has finished I doubt that will be the case - maybe we can get a more concrete answer from him.

I know some progress on #761 was made offline about some 8 or so months ago - but was never pushed upstream. If I do recall correctly the fix fixed only a subset of issues associated with #750 and we couldn't figure out how to solve the rest. @tcmoore3 might remember more details about this.

[vyasr]

@bdice I haven't really followed the discussions around S(q) since there have been so many other interested parties, but looking at #750 I am a little concerned. If a user provides a non-cubic box, do we have any guarantees that we're generating the correct results right now? If not, I would recommend disabling S(q) for non-cubic boxes (can use some numerically approximate check to verify this internally) until one of the PRs addressing these issues gets merged in. Not everyone will do the same thorough analysis that @tcmoore3 did in #750, and while API instability is fine in a released version if we document it as such, silently generating invalid values is definitely not (especially if we know the bugs exist and the fixes have been stalled for a year at this point).

CC @tommy-waltmann in case this affects your release plan.

[bdice]

@vyasr I agree that the current behavior of silently giving incorrect results is not good. As I recall, the diffraction image is qualitatively almost-right for non-cubic boxes, up to an affine transformation of the image, which turns out to be somewhat useful (even if incorrect) if you’re looking at the image with an interactive 3D linked view in OVITO. However, the k-values on the axes and matrix of k-vectors definitely would not correspond to the image’s peaks unless that transformation is applied to the image. I had hoped this would get fixed by one of the several developers who have attempted a fix over the last several releases ago but it has stalled. In light of that, failing explicitly for noncubic boxes would be a good way to prevent users from interpreting the system incorrectly.

[vyasr]

@tommy-waltmann do you think you could do that instead before making the next release? I think that's the responsible thing to do for our users until someone actually fixes the underlying problem. It might also encourage someone to actually fix the problem instead of relying on the current "almost right" behavior, which would be ideal.

[tommy-waltmann]

Yes, I can do that. It will likely push the release back to next week, but that's not a big deal.