Ch/kltransform fsorder - #102
Conversation
(cherry picked from commit 3af5152218f19c271b0e26d7e1d8e6523f6f8772)
|
This pull request introduces 1 alert when merging de682ea into 94f0ece - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging b7733b1 into 94f0ece - view on LGTM.com new alerts:
|
sjforeman
left a comment
sjforeman
left a comment
There was a problem hiding this comment.
Only a few minor things to look at...
| ind = np.where(evals < self.foreground_threshold) | ||
|
|
||
| # Construct evextra dictionary (holding foreground ratio) | ||
| evextra = {"ac": ac, "f_evals": evals.copy()} |
There was a problem hiding this comment.
Should we consider using sf_evals for S/F transform and fs_evals for F/S transform, to make it more explicit which eigenvalues are being stored?
| if self.diagonalisation_order == "sf": | ||
| f_evals = evextra["f_evals"] | ||
| else: | ||
| f_evals = evextra["f_evals"][::-1] |
There was a problem hiding this comment.
Why do you reverse the order for F/S eigenvalues? To be consistent with the single-KL F/S case, shouldn't the order be left as it is?
|
Hey @sjforeman , I addressed your requests, the issue right now is that when simulating a pathfinder sized telescope the transform is not able to complete in the S/N step (matrix not positive definite). So this probably requires more research time before a final merge. |
| else: | ||
| f_evals = evextra["f_evals"] | ||
|
|
||
| f.create_dataset("f_evals", data=f_evals) |
There was a problem hiding this comment.
This if-else doesn't do anything...
There was a problem hiding this comment.
ah ja missed that one.. thanks.
Let's try it again after the new cedar CHIME environment is ready. I think pathfinder-sized telescopes were working fine for me, but I manually upgraded the version of scipy in my virtualenv. The new standard environment should have newer packages, so maybe that will solve your matrix issue. |
|
Alright sounds good! |
|
@cahofer I wrote down on Friday that you were going to run a quick test against the new CHIME env. Let us know when you get around to that one, but I don't think it's a rush. |
|
I'm going to convert this one to a draft until it's closer to be ready to merge. |
|
@cahofer Just wanted to follow up on this one. Did you manage to get any tests running through? |
|
uuuggh I am pushing through the very lasts bits of my thesis I will put this on hold until after the thesis is submitted! sorry! |
3 participants
Allow F/S diagonalisation order in
kltransform.KLTransformKLtransformcan be configured over the 'diagonalisation_order' config property. Accordingly, either high S/F and low F/S eigenvalues are selected.