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

Global coherence #573

Merged
merged 21 commits into from
Jun 6, 2024
Merged

Global coherence #573

merged 21 commits into from
Jun 6, 2024

Conversation

alexkjames
Copy link
Contributor

Adds global coherence functionality to pyleoclim. Includes a .plot and .signif_test method.

Built on top of wavelet_coherence.

alexkjames and others added 12 commits June 4, 2024 12:44
* pin all python versions to 3.11.0 (#567)

* pin all python versions to 3.11.0

* Update .travis.yml

* Update rtd_env.yml

* Update environment.yml

* Update environment.yml

* Update testmaster.yml

* pin to 3.11.0

* Update .readthedocs.yaml (#568)

* Mcpca doc (#570)

* ensemblegeoseries docstring fix

* ensmultivardecomp doc string fix

* mgs docstrings

* ms docstrings

* mvd docstrings

* doc fixes

* tick version (#571)

* tick version

* add requests to setup.py

* verbose doc fix

---------

Co-authored-by: Alexander James <akeenjames@gmail.com>

---------

Co-authored-by: Jordan Landers <10099739+jordanplanders@users.noreply.github.com>
Co-authored-by: Julien Emile-Geay <CommonClimate@users.noreply.github.com>
@alexkjames alexkjames marked this pull request as ready for review June 5, 2024 22:21
Copy link
Collaborator

@CommonClimate CommonClimate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! A few minor changes to take it to 11.

  1. On the code side, since Series.global_coherence() is built on top of wavelet_coherence(), I recommend having an optional parameter called coh, which would None by default, but would allow you to pass a coherence object from which the average would be taken.

  2. The gcoh.plot() command should have a label on the right-hand side that says "coherence" (or "global coherence") by default, which could be controlled by a parameter called ylabel_rhs (similarly, need a ylabel_lhs = PSD by default)

  3. On the docstring side, I recommend that the docstring for Series.global_coherence() does something with gcoh (e.g. plot it). Otherwise, it's a very frustrating docstring to read because it ends in a blind corner.

@alexkjames
Copy link
Contributor Author

Nice work! A few minor changes to take it to 11.

1. On the code side, since `Series.global_coherence()` is built on top of wavelet_coherence(), I recommend having an optional parameter called `coh`, which would `None` by default, but would allow you to pass a coherence object from which the average would be taken.

2. The gcoh.plot() command should have a label on the right-hand side that says "coherence" (or "global coherence") by default, which could be controlled by a parameter called `ylabel_rhs` (similarly, need a `ylabel_lhs = PSD` by default)

3. On the docstring side, I recommend that the docstring for  `Series.global_coherence()` does something with `gcoh` (e.g. plot it). Otherwise, it's a very frustrating docstring to read because it ends in a blind corner.

The latest commits should address these:

  1. Added this
  2. There is an exposed argument for this, I updated the default and added an exposed argument for the left hand side label, as well as the x axis.
  3. Added this

@alexkjames alexkjames requested a review from CommonClimate June 6, 2024 01:23
Copy link
Collaborator

@CommonClimate CommonClimate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go

@CommonClimate CommonClimate merged commit a6d2d66 into master Jun 6, 2024
1 check passed
@alexkjames alexkjames deleted the cross_spectral_coherence branch June 6, 2024 17:46
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.

2 participants