Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forward args from apply_and_annotate to apply_test #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

StefRe
Copy link

@StefRe StefRe commented May 31, 2022

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.

so you and use the shortcut convenience apply_and_annotate instead of
apply_test and annotate individually
@trevismd
Copy link
Owner

trevismd commented Jun 20, 2023

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
Copy link
Author

StefRe commented Jun 20, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants