trevismd / statannotations

add statistical significance annotations on seaborn plots. Further development of statannot, with bugfixes, new features, and a different API.
Other
624 stars 67 forks source link

Forward args from apply_and_annotate to apply_test #60

Open StefRe opened 2 years ago

StefRe commented 2 years ago

so you can use the shortcut convenience apply_and_annotate method with parameters instead of calling apply_test and annotate individually.
See also https://stackoverflow.com/a/72445947/3944322.

trevismd commented 1 year ago

Hello @StefRe , Sorry it took me a over a year to reply here. Time flies. Thanks for the PR! The problem here is that both apply_test and annotate have parameters. Choosing that apply_and_annotate should take arguments only for apply_test can be confusing too. We could do something like def apply_and_annotate(apply_params, annotate_params) but is it really more convenient then .apply_test(...apply_params).annotate(...annotate_params)?

I think rather keep the simple apply_and_annotate when you're happy with the configured values and defaults.

StefRe commented 1 year ago

No problem - I just came across it when answering the SO question and already forgot about it.

It's not a necessity, but once we have apply_test and annotate and then the combined function apply_and_annotate I thought it would be logical to assume that the combined function took the same args as the individual functions.

The problem here is that both apply_test and annotate have parameters.

Oh, I hadn't considered that. In this particular case, however, only one of them takes var-key args, so the solution would be straightforward here:

    def apply_and_annotate(self, line_offset=None, line_offset_to_group=None, num_comparisons='auto', **stats_params):
        """Applies a configured statistical test and annotates the plot"""

        self.apply_test(num_comparisons, **stats_params)
        return self.annotate(line_offset, line_offset_to_group)

But again, no big deal for me - you can leave it as is and close the PR. It was more a spontaneous change request than a real necessity.