Unclear default value for normalize kwarg in freud.density.RDF

[jennyfothergill]

Describe the bug
The help for freud.density.RDF says this about the normalize kwarg:
| normalize (bool, optional):
| Scale the RDF values by
| :math:\frac{N_{query\_points}}{N_{query\_points}-1}. This
| argument primarily exists to deal with standard RDF calculations
| where no special query_points or neighbors are provided,
| but where the number of query_points is small enough that the
| long-ranged limit of :math:g(r) deviates significantly from
| :math:1. It should not be used if :code:query_points is
| provided as a different set of points, or if unusual query
| arguments are provided to :meth:~.compute, specifically if
| :codeexclude_ii is set to :code:False. This normalization is
| not meaningful in such cases and will simply convolute the data.

I think the default value is True, but this could be more clear. Please forgive me if this is obvious or I have just missed it somewhere!

To Reproduce
help(freud.density.RDF)

Error output
n/a

System configuration (please complete the following information):

• OS: OSX 10.14.6 (18G103)
• Version of Python 3.7.6
• Version of freud '2.2.0'

Additional context
n/a

[vyasr]

Actually, the default is False, and the docstring should be updated to include the usual indicator Default value: False. However, I can also see that the naming is a bit confusing in light of what we're discussing on #396. I'm wondering if we instead need to add a string argument normalization_mode. The mode would default to 'infer', in which case it would handle the two obvious cases discussed in #396 (query_points either being identical to or completely disjoint from points). Setting normalize to 'none' would completely remove normalization by particle number, in which case it would be up to the user to compute the normalization factor. Setting normalize to 'finite_size' would apply the current normalization (N-1/N), which is really only relevant for small systems. @jennyfothergill does that make sense? Then we would update the docstrings accordingly (including with defaults) and deprecate the current normalize argument, marking it for removal in freud 3.0 (to be replaced by the normalization_mode).

[bdice]

@vyasr Does this need to be API-breaking? I'd prefer to just handle a third option for normalize instead of creating a new argument named normalization_mode. Then True and False would be removed in 3.0.

True or 'finite_size'  # Handled the same way
False or 'infer'  # Handled the same way
'disabled'  # New option

[vyasr]

I think that's fine, as long as we remove documentation of the boolean arguments (and replace with the new keywords) and raise DeprecationWarning whenever booleans are provided. My primary concern is that in the new proposal False and 'disabled' would mean different things (using your terminology), which is very confusing.

[bdice]

Sounds like a good plan. We can list all values / replacements in the DeprecationWarning to make it clear.

[vyasr]

@bdice since we're thinking about making 3.0 due to #738, this should also be done for that (at least renaming the existing options, whether or not the new normalization features are added by then).

[vyasr]

@tommy-waltmann to speed up 3.0 I would recommend just doing the renaming of existing normalizations without implementing new normalization solutions.

[tommy-waltmann]

Ok, then the argument will be named normalization_mode and the acceptable options will be finite_size or infer (default), following @bdice 's suggestion. That way, if we want to add more normalization modes in the future, we can do that and not have API breakages.

[tommy-waltmann]

closed via #1013