-
Notifications
You must be signed in to change notification settings - Fork 26
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
Improvements to gwdetchar-lasso-correlation and gwpy-0.12.0 compatibility #134
Improvements to gwdetchar-lasso-correlation and gwpy-0.12.0 compatibility #134
Conversation
only call function if we need to use the result
just taken from `gwdetchar-lasso-correlation` and optimised a wee bit
and some simplification of parsing results of Lasso fit
Pull Request Test Coverage Report for Build 306
💛 - Coveralls |
The reduction in coverage is purely down to a new module being added that doesn't have unit tests (yet). We (me and the CSUF team) should also work to migrate more code out of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@duncanmmacleod I'm a big fan of all of this abstraction into gwdetchar.lasso
. From a gwdetchar-lasso-correlation
perspective, this all looks great to me. From the gwpy
side of things, I'm not as informed. Can you explain in more detail what you are referring to when you say that the results are almost the same, since I'm unable to generate a sample page? And what upstream source are you referring to with the OrderedDict
s?
@macedo22, sorry for the delay in replying. The change to the results has nothing to do with gwpy, but to a subtle change in the ordering of the channels. The posted example was generated using the following command (run from any machine on the LIGO-Livingston LDAS system):
The results from an unmodified code are here. If you apply only the following patch: diff --git a/bin/gwdetchar-lasso-correlation b/bin/gwdetchar-lasso-correlation
index 679999f..207f484 100755
--- a/bin/gwdetchar-lasso-correlation
+++ b/bin/gwdetchar-lasso-correlation
@@ -27,6 +27,7 @@ from subprocess import call
import tempfile
import atexit
import shutil
+from collections import OrderedDict
from math import (isnan, isinf, log, log10)
import numpy
@@ -264,8 +265,8 @@ auxdata = TimeSeriesDict.get(
observatory=args.ifo[0], pad=0, **io_kw)
# -- removes flat data to be re-introdused later
-flatdata = dict()
-gooddata = dict()
+flatdata = OrderedDict()
+gooddata = OrderedDict()
for k, ts in auxdata.items():
flat = ts.value.min() == ts.value.max() the results are slightly different, see here. The use of The results from after applying all of the changes from this PR are here, and match the modified results above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@duncanmmacleod That's so interesting, but I think you're right. The fit probably just changes because it is iterating through and updating the model over a different order of auxiliary channels. This all seems good to me
This PR makes some general improvements to
gwdetchar-lasso-correlation
inspired when trying to update the plotting interface to begwpy-0.12.0
compatible.The changes are almost backwards compatible, meaning that they almost exactly reproduce the old results, however they are subtly different due to a change in the channel ordering (use of
OrderedDict
compared todict
). The results exactly match if the upstream code is patched to useOrderedDict
(with no further changes).This is the companion PR to #118, so should be merged immediately after that one.
cc @macedo22