rs-station / reciprocalspaceship

Tools for exploring reciprocal space
https://rs-station.github.io/reciprocalspaceship/
MIT License
28 stars 12 forks source link

`DataSet.stack_anomalous()` should accept suffixes as alternatives to full column specification #100

Closed dennisbrookner closed 2 years ago

dennisbrookner commented 2 years ago

Related to this (and hopefully not compounding the issue) would it make sense for DataSet.stack_anomalous() to take something like plus_suffix and minus_suffix arguments, as alternatives for plus_labels and minus_labels?

Definitely not critical, but would a) occasionally save some typing and b) be internally consistent with the way the defaults work (without breaking previous code), e.g. plus_suffix="(+)" / minus_suffix="(-)"

(This also seems like an easy enough change that I could try to tackle it myself, with blessing?)

Originally posted by @dennisbrookner in https://github.com/Hekstra-Lab/reciprocalspaceship/issues/99#issuecomment-936516668

JBGreisman commented 2 years ago

I think this seems like a reasonable extension to DataSet.stack_anomalous(). Is the plan to have plus_labels and minus_labels take precedent if both sets are provided?

I also think that it makes most sense to use the argument suffixes=("(+)", "(-)") rather than plus_suffix/minus_suffix. I think that is most consistent with the pandas-style syntax, and is analogous to the call signature of unstack_anomalous().

Feel free to tackle this, and reach out if you run into trouble. Looking forward to your PR!

dennisbrookner commented 2 years ago

Yes, I think having plus_labels and minus_labels take precedent is the best approach here (and probably safest for backwards compatibility). So the call would be:

DataSet.stack_anomalous(suffixes=("(+)", "(-)"), plus_labels=None, minus_labels=None)

and the suffixes argument (either the default or something different) is only checked when plus_labels and minus_labels are both None.

JBGreisman commented 2 years ago

I'm inclined to flip that order to:

def stack_anomalous(plus_labels=None, minus_labels=None, suffixes=("(+)", "(-)")):

Otherwise, you break backwards compatibility in cases where plus_labels and minus_labels are provided as positional arguments.