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

Adding Karhunen-Loeve #41

Merged
merged 5 commits into from
Jul 24, 2018
Merged

Adding Karhunen-Loeve #41

merged 5 commits into from
Jul 24, 2018

Conversation

GillesOrban
Copy link
Contributor

Python implementation of KL basis based on Cannon 1996 and resp. IDL and Yorick (YAO) codes.

@codecov
Copy link

codecov bot commented Jul 17, 2018

Codecov Report

Merging #41 into master will decrease coverage by 0.26%.
The diff coverage is 92.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
- Coverage   94.06%   93.79%   -0.27%     
==========================================
  Files          24       25       +1     
  Lines        1011     1241     +230     
==========================================
+ Hits          951     1164     +213     
- Misses         60       77      +17
Impacted Files Coverage Δ
aotools/functions/__init__.py 100% <ø> (ø) ⬆️
aotools/functions/karhunenLoeve.py 92.57% <92.57%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b64533...2e6554e. Read the comment docs.

@andrewpaulreeves
Copy link
Contributor

Looks great! This would be really useful to have.

There seems to be a problem with Python 2 however and it would be nice to fix this before merging. This is the output from Travis:

======================================================================
ERROR: test_KL.test_make
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda3/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/AOtools/aotools/test/test_KL.py", line 5, in test_make
    kl, _, _, _ = KL.make_kl(150, 128, ri=0.2, stf='kolmogorov')
  File "/home/travis/build/AOtools/aotools/aotools/functions/karhunenLoeve.py", line 635, in make_kl
    stf=stf, outerscale=outerscale)
  File "/home/travis/build/AOtools/aotools/aotools/functions/karhunenLoeve.py", line 374, in gkl_basis
    evals, nord, npo, oord, rabas = gkl_fcom(ri, kers, nfunc)
  File "/home/travis/build/AOtools/aotools/aotools/functions/karhunenLoeve.py", line 252, in gkl_fcom
    newev, vs = np.linalg.eigh(fktom * kers[:, :, nxt])
IndexError: index 200 is out of bounds for axis 2 with size 200
-------------------- >> begin captured stdout << ---------------------
('note, for this size basis, radial discretization on ', 40)
points is finer than necessary - it should work, but you 
could take a smaller 'nr' without loss of accuracy
--------------------- >> end captured stdout << ----------------------

I've had a quick look at the code and can't see anything obvious that will break it, the main difference that I can think would have caused this is a difference in float division - in Python 2 a/b = Integer division and in Python 3 it is Floating point division. This could maybe cause an index to be rounded up at some point to a value that is too large? Thats the first thing that springs to mind anyway. Do you have a convenient setup to do some python 2 debugging?

@GillesOrban
Copy link
Contributor Author

Hi! I ve seen it but didn't have time to fix it yet. Indeed, not obvious at first sight (agree with you, probably some integer division issue).
As soon as I have time to setup a python 2 env and debug, I ll do.
Regarding tests, this might be more tedious. I don't guarantee immediate improvement...

@matthewtownson
Copy link
Member

This is hugely useful, thanks! Will merge it in now.

@matthewtownson matthewtownson merged commit 14bb56b into AOtools:master Jul 24, 2018
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

3 participants