-
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
Command-line module for gwdetchar.scattering #298
Conversation
ff2c089
to
23bcc31
Compare
f278f27
to
84a42ae
Compare
cc @jrsmith02, @andrew-lundgren, @duncanmmacleod For testing and benchmarking, I've run this module on a recent time with known scattering. The command line used is like so: $ python -m gwdetchar.scattering -i L1 1238510178 This identified 1 channel whose predicted fringe frequency (4th harmonic) strays above 15 Hz, which is This process completed in 18.858 seconds. |
74091e6
to
d820777
Compare
I have no particular objections about this change, but I think having two different command-line interfaces to scattering:
might be confusing. I, at least, would expect those to do the same thing, but clearly they will not. As long as this is documented well (in the |
This looks great to me, not to be greedy, but one thing we use quite frequently is the ability to overlay the fringes on the time-frequency plot. Might the user optionally choose overlay, to get one plot with both the omega scan and the predicted fringe frequencies shown together? (In this case, the blue line color should be excluded from the time series curves.) Other than that, thank you so much, I love this work and will use it often! |
97cfd94
to
04787b0
Compare
@duncanmmacleod, that's definitely fair, I've added short descriptions to the docstring of both utilities that informs the user what each is used for. @jrsmith02, as discussed on slack, we'll add in the overlay feature for a future release, and go with what we already have for now. |
04787b0
to
f0d1f58
Compare
@jrsmith02, I changed my mind and coded up the overlay plot now, example is here: Since plotting eats up neither memory nor time in this case, I've decided to just make both plots for every channel whose scattering fringes are predicted to cross above 15 Hz. |
b138269
to
7e7f9a1
Compare
a5fe2fb
to
e105e47
Compare
Codecov Report
@@ Coverage Diff @@
## master #298 +/- ##
==========================================
+ Coverage 95.53% 95.67% +0.14%
==========================================
Files 21 23 +2
Lines 1522 1571 +49
==========================================
+ Hits 1454 1503 +49
Misses 68 68
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #298 +/- ##
==========================================
+ Coverage 95.53% 95.67% +0.13%
==========================================
Files 21 23 +2
Lines 1522 1569 +47
==========================================
+ Hits 1454 1501 +47
Misses 68 68
Continue to review full report at Codecov.
|
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.
Just a few minor things, in general this will be very useful.
Additional change: please add python -m gwdetchar.scattering --help
or similar to the CI configuration.
bin/gwdetchar-scattering
Outdated
|
||
This tool identifies time segments when evidence for scattering is strong, | ||
to compare projected fringes against spectrogram measurements for a specific | ||
time please use the command-line module: `python gwdetchar.scattering --help` |
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.
time please use the command-line module: `python gwdetchar.scattering --help` | |
time please use the command-line module: `python -m gwdetchar.scattering --help` |
gwdetchar/scattering/__main__.py
Outdated
output = os.path.join( | ||
args.output_dir, | ||
'%s-%s-%s-{}.png' % ( | ||
channel.replace(':', '-'), gps, args.duration) |
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.
I think this needs to be a little more specific:
channel.replace(':', '-'), gps, args.duration) | |
channel.replace('-', '_').replace(':', '-', 1), gps, args.duration) |
gwdetchar/scattering/core.py
Outdated
velocity.override_unit('m/s') # just so multiplication works | ||
velocity = type(series)(numpy.zeros(series.size)) | ||
velocity.__array_finalize__(series) | ||
velocity[:] = savgol_filter(series.value, 5, 2, deriv=1) |
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.
Either use numpy.empty
or
velocity = savgol_filter(series.value, 5, 2, deriv=1).view(type(series))
velocity.__array_finalize__(series)
gwdetchar/scattering/plot.py
Outdated
@@ -0,0 +1,151 @@ | |||
# coding=utf-8 | |||
# Copyright (C) Alex Urban (2018-) |
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.
# Copyright (C) Alex Urban (2018-) | |
# Copyright (C) Alex Urban (2018-2019) |
gwdetchar/scattering/plot.py
Outdated
|
||
def _format_timeseries(ax, gps, fringe, multipliers=(1, 2, 4, 8), | ||
linewidth=1, thresh=None): | ||
"""Helper tool to format a `~gwpy.timeseries.TimeSeries plot axis |
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.
"""Helper tool to format a `~gwpy.timeseries.TimeSeries plot axis | |
"""Helper tool to format a `~gwpy.timeseries.TimeSeries` plot axis |
Added gwdetchar.scattering.plot module Added unit test for scattering plot Use a Savitzky-Golay filter to take derivatives Document separate command-line interfaces for scattering
f3489bd
to
c437a39
Compare
c437a39
to
f523920
Compare
Thanks for reviewing this @duncanmmacleod, I've addressed your comments and added a coverage test for |
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.
LGTM, thanks for addressing my comments.
This PR implements a command-line module for scattering investigations, which can be invoked via
Changes include:
gwdetchar.scattering
into a proper sub-module with its own__init__.py
gwdetchar/scattering/__main__.py
, which projects scattering fringe frequencies of all the default optic motion channelsgwdetchar-scattering
outputgwdetchar.lasso.tests
This fixes #296.
cc @duncanmmacleod, @jrsmith02, @andrew-lundgren