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

Added Extended Eof (EEof) #25

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

arulalant
Copy link

Dear Dr. Andrew Dawson,

My friend Dileep and myself added support for "Extended Empirical Orthogonal Function Analysis" as we discussed so long back with respect to issue #12 .

We added EEof class methods only which are all we tested using cdms module (via UV-CDAT) and results are getting correct. Other Eof class methods will be used by the users in case of need, because we inherited Eof super class to EEof sub class, in case of need by the users.

I never used iris module, so I couldn't add full fledged support for it. But you may fix it where I mentioned TODO tag (create time axis iris syntax) in iris module's EEof Class constructor.

We tested EEof analysis by reproducing the same results given in the paper "Kikuchi K, Wang B, Kajikawa Y (2011), Bimodal representation of the tropical intraseasonal oscillation. Clim Dyn. doi:10.1007/s00382-011-1159-1 Link"

We used "A. Hannachi, 2004, A Primer for EOF Analysis of Climate Data, Department of Meteorology, University of Reading, Reading RG6 6BB, U.K. Link" documentation to reconstruct input matrix as extended input matrix to conduct Eof analysis on it, with respect to input lag window.

With Regards,
Arulalan.T

@ajdawson
Copy link
Owner

First off, thanks for making the PR, great to have more contributors. I just wanted to leave a note to tell you I have seen this, but may not get round to a full review for a little while!

@ajdawson
Copy link
Owner

Sorry for the delay. I have a couple of structural issues first off:

  1. I think the extended EOF classes should live in their own namespace (just like multivariate EOFS), I'd suggest eofs.extended.
  2. Since the Iris class for extended EOFs doesn't function, you could just remove it. It can be added later in a separate pull request (by someone else even).

I'm also a little concerned about the use of inheritance to define the extended EOF solvers. Are you absolutely sure that every method of the Eof base class will work and provide meaningful results when inherited by the EEof classes? I'm not fully convinced that this approach is suitable.

Do you have any thoughts about how we might test this functionality. Do you have a (small) reference data set that can be used in automated tests to ensure the behaviour doesn't change?

@arulalant
Copy link
Author

Sorry for the delayed response.
Now I am able to understand iris module as well.
Let me work on this to include iris capability. I will take consideration of your suggestions.
Thanks.

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