Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

ENH: Add MLS method #3351

Merged
merged 1 commit into from May 28, 2014

Conversation

Projects
None yet
6 participants
Member

larsoner commented Feb 19, 2014

This adds MLS calculation. Not 100% sure waveforms is the correct place for it, but it is a waveform, so perhaps it is.

Cython was about a factor of 10x faster than Python, so I used that.

Owner

rgommers commented Feb 19, 2014

TravisCI run was OK, except that the 2.7 tests timed out.

Member

larsoner commented Feb 19, 2014

Does that mean the test was too slow (is there some time limit), or that
it's broken? I'd be surprised by the latter since I develop on 2.7, but I
can take a look.

Coverage Status

Coverage remained the same when pulling 74bfb79 on Eric89GXL:add-mls into 340e4e2 on scipy:master.

Member

larsoner commented Feb 19, 2014

Never mind -- I see that there is a 50-minute time limit for Travis tests. I couldn't tell if the timeout was really due to my change, so I rebased to most recent master just to make sure (even though I based mine off of a master from about 12 hours ago). Without any other changes, the build finished this time in under 40 minutes.

It looks like some of the other builds recently have come pretty close to hitting the 50-minute limit, too:

https://travis-ci.org/scipy/scipy/builds/19140627
https://travis-ci.org/scipy/scipy/builds/19139353
https://travis-ci.org/scipy/scipy/builds/19138862

So this might be an issue with the scipy testing scheme more than this PR specifically.

But just to be safe, I reduced the number of test cases run so it should complete ~4x faster. I also made tests slightly more comprehensive. I have to catch some sleep now, hopefully Travis is happy with the latest commit that does these things, too!

Member

larsoner commented Feb 19, 2014

By the way, this is the first PR where I'm building Cython (more or less) from scratch. I'd love to get feedback on any ways the code could be made more efficient if someone has time to comment on that -- I'm not 100% confident I've made full use of C datatyping, but I think I have. If people are too busy to think about it, though, no problem.

@larsoner larsoner commented on an outdated diff Feb 19, 2014

scipy/signal/_mls.pyx
+
+ n_taps = taps.size
+ c_n_bits = n_bits
+ n_out = (2**n_bits) - 1
+ seq = np.empty(n_out, dtype=np.float64)
+ idx = 0
+ for ii in xrange(n_out):
+ feedback = shift_register[idx]
+ seq[ii] = feedback
+ for ti in xrange(n_taps):
+ feedback ^= shift_register[(taps[ti] + idx) % c_n_bits]
+ shift_register[idx] = feedback
+ idx = (idx + 1) % c_n_bits
+ # convert from "binary" to -1's and +1's
+ seq *= 2.
+ seq -= 1.
@larsoner

larsoner Feb 19, 2014

Member

Note that I could move these up to line 85 as:

seq[ii] = 2*feedback - 1

But then I thought this might actually end up being faster, since it's possible that SIMD instructions could more easily be used with lines like seq *= 2 than the more challenging loop above. Not 100% sure though...

Coverage Status

Coverage remained the same when pulling 38b1cb7 on Eric89GXL:add-mls into 340e4e2 on scipy:master.

Coverage Status

Coverage remained the same when pulling 38b1cb7 on Eric89GXL:add-mls into 340e4e2 on scipy:master.

Member

larsoner commented Feb 19, 2014

Name changed to max_len_seq from listserv suggestion.

Coverage Status

Coverage remained the same when pulling 75f8d6e on Eric89GXL:add-mls into 340e4e2 on scipy:master.

@pv pv added the PR label Feb 19, 2014

Member

larsoner commented Feb 19, 2014

Used Cython annotation to figure out that I wasn't typing my variables appropriately. Now the whole loop should be in C, which is nice. Should be ready for review whenever people have time.

Coverage Status

Coverage remained the same when pulling 7614cf0 on Eric89GXL:add-mls into 340e4e2 on scipy:master.

Coverage Status

Coverage remained the same when pulling 7614cf0 on Eric89GXL:add-mls into 340e4e2 on scipy:master.

Coverage Status

Coverage remained the same when pulling b48eeac on Eric89GXL:add-mls into 340e4e2 on scipy:master.

Member

larsoner commented Feb 23, 2014

I would love to have this in 0.14. It's fairly simple, so it seems at least plausible. It's not necessary if it seems like it would be rushed, though. Not sure what the release timeline is like...

Owner

rgommers commented Feb 23, 2014

Timeline is first beta straight after branching, so in a day or two I hope. Then a first release candidate 2 weeks later and the final release (depending on issues found) 2 weeks after that.

Member

larsoner commented Feb 23, 2014

Hmm, that is pretty quick. This should be good to go, but it hasn't really been reviewed yet so I understand if it's not plausible.

Owner

rgommers commented Feb 23, 2014

This needs a rebase.

Coverage Status

Coverage remained the same when pulling 1d41593 on Eric89GXL:add-mls into dfd2e2b on scipy:master.

Coverage Status

Coverage remained the same when pulling 1d41593 on Eric89GXL:add-mls into dfd2e2b on scipy:master.

Member

larsoner commented Feb 23, 2014

Rebased.

Member

larsoner commented Feb 28, 2014

I'm assuming that this probably won't make 0.14, then?

Owner

rgommers commented Feb 28, 2014

Sorry, don't think so. Just yesterday we had to fix the Windows builds for some unexpectedly broken Cython code, so I'm hesitant to merge this at the very last moment. Also still needs someone to look it over in detail.

Member

larsoner commented Feb 28, 2014

Sounds reasonable. Thanks for the info.

Member

larsoner commented Apr 23, 2014

@rgommers do you know any signal processing folks who might be qualified to look at this? If not, I can recruit someone to take a look.

Member

WarrenWeckesser commented Apr 23, 2014

@eric89gxl: I'm not familiar with MLS, but it's always fun to learn something new. :) I'll look into it and this PR, and try to provide some feedback by the weekend.

Member

larsoner commented Apr 23, 2014

Great, thanks!

Member

WarrenWeckesser commented Apr 25, 2014

Eric,

To figure out what MLS is all about, I ended up creating my own implementation. See this gist: https://gist.github.com/WarrenWeckesser/11304418. This is not meant to be a replacement for your cython version; cython is almost certainly the right way to go. I created my own version to check my understanding, to not have cython in my initial edit-build-test loop, and to have an alternative API for discussion. The good news is your output matches mine (modulo rotating the periodic sequence, and converting {0,1} to {1, -1}).

Before getting into implementation details, l have some suggestions for the API. The signature of my version is a bit different than yours:

def mls_lfilter(n_bits, state=None, taps=None, n_samples=None)

state is just my version of your seeds argument. taps allows a different polynomial to be used to generate the MLS. The most significant difference is n_samples. It tells the function to return only that many samples, instead of the full 2**n_bits - 1 samples. My version returns the array of samples and the final state, so it can be reused in subsequent calls. The idea is that, instead of getting all 2**n_bits - 1 at once, you can get blocks of samples in batches. E.g.

m = 50
# Get the first `m` samples.
y, state = mls_lfilter(12, n_samples=m)
# Get the next `m` samples.
y, state = mls_lfilter(12, n_samples=m, state=state)
# Etc...

For small to moderate n_bits, this isn't really necessary, but it could avoid memory problems when generating really long sequences. What do you think?

In my version, I left the samples as the binary digits 0 and 1. It should be changed to return 1 and -1 by default, but it might be nice to have the option of returning the binary digits instead. How about adding such an option to max_len_seq? Also, did you have a good reason for returning a floating point array? I think an array with type np.int8 makes more sense.

I have more suggestions, but they are relatively minor. Let's get the main API figured out, then get into bike-shedding. :)

Member

larsoner commented Apr 25, 2014

Awesome -- makes a lot of sense to me (and good news about the match). I hadn't considered memory consumption, but if we support up to 32 bits by default, that would indeed be a lot of data! I also like being able to pass in different coefficients if one wants, since I (arbitrarily) sub-selected defaults to populate _mls_taps.

An output flag would be fine, but I'd prefer to avoid it to avoid building up kwargs. What do you think about just passing back [1, 0] / np.int8, since this would be a memory-efficient representation? From an output of [1, 0] of type np.int8, getting to the [+1, -1] requires two trivial arithemetic operations as out * 2. - 1.. Avoiding the extra kwarg might be worth such an extra operation at the user end. WDYT? The ideal output representation in my mind would actually be boolean array, but I tried for quite a while to get it to work in Cython and failed -- if you know how to get that to work, that would be my ideal choice.

For what it's worth, my original motivation behind having the output be a float is that these are often used in calculations of circular cross-correlations with a response to the MLS function, which I assume would make having a float64 datatype convenient (no re-casting). But in retrospect I don't find this as compelling as the memory/compactness argument.

I don't like the name mls_lfilter as much as max_len_seq, though -- does the former fit better in scipy parlance? I don't usually think of MLS calculation as a form of linear filtering, even though I probably should. I can also see how the name mls_lfilter might make more sense in the context of needing repeated calls to the function to obtain the final sequence, so if you like it better I can easily live with it.

It would be great to converge on the API using your code, and then make the final changes in Cython -- that way, I can add the pure Python code in the comments so it's clear how it's supposed to work, and future editors can more easily tweak/test, etc. And thanks for the thorough review!

Member

WarrenWeckesser commented Apr 25, 2014

Sorry, I didn't mean to suggest that the function should be called mls_lfilter. I gave my function a different name so I could import mine and yours without a namespace prefix. max_len_seq is fine.

More later...

Member

WarrenWeckesser commented Apr 26, 2014

I think returning sequences of {0,1} (with data type np.int8) is fine. If it turns out there is interest in an option for returning {1, -1}, it would be easy to add later.

Here's a proposed API. (The change of n_bits and seeds to nbits and seed is getting down to bikeshed-level tweaks. I couldn't find any precedent in the scipy code [other than a benchmark and a test or two] where n_<stuff> was used. My preference is either nbits or num_bits.)

Signature

def max_len_seq(nbits, seed=None, taps=None, nsamples=None):

Parameters

nbits is the number of bits in the shift register that is used to generate the sequence.

If given, seed must be a 1-d sequence with length nbits with values in {0, 1}. seed is used as the initial state of the shift register that generates the maximum length sequence. If seed is not given, the initial state is all 1s.

taps must be a 1-d sequence of integers between 0 and nbits. These are the powers in the primitive polynomial with nonzero coefficients. For example, for the polynomial

  g(x) = x^12 + x^7 + x^4 + x^3 + 1,

taps is [12, 7, 4, 3, 0]. Because the coefficients of x^nbits and x^0 are always 1, these may be left out of the taps list. E.g. for this example, taps=[7, 4, 3] is allowed. The values in taps do not have to be in any specific order. E.g. taps=[3, 4, 7] is allowed.

A default value for taps is provided for nbits up to M. (In the current implementation of max_len_seq, M is 32, but the data in the paper by Stahnke [1] could be used to increase M to 168.) If nbits exceeds M, taps must be given.

nsamples is the length of the sequence to return. If not given, the full sequence with length 2**nbits - 1 is returned. I don't see a reason to constrain nsamples, so any nonnegative value should work. (I prefer edge cases to "just work". If nsamples=0, the returned sequence should have length 0.)

Return Value

The function returns (seq, state):

seq is a numpy array of 0s and 1s with type np.int8.

state is a numpy array of 0s and 1s with length nbits. It may be used as the seed value in a call to max_len_seq to continute the sequence. For example,

  # Generate the first 50 samples.
  seq, state = max_len_seq(10, nsamples=50)
  # Generate the next 50 samples.
  seq, state = max_len_seq(10, nsamples=50, seed=state)

[1] Stahnke, W., "Primitive Binary Polynomials", Mathematics of Computation,
Vol. 27, No. 124, October 1973.

Member

larsoner commented Apr 26, 2014

This sounds great. I'll get to work on it hopefully Monday. One more thing
to note is that we should probably protect the user from errantly supplying
zeros as the initial state, as this leads to the trivial (but still
computed) zero output. While some people might want this use case, that's
probably more rare than someone mistakenly falling into it. Or if the state
is all zeros, we could just return zeros immediately with a warning. Up to
you.

I'm okay with adding more values, but I'd rather stick with the 32 that are
in there. We can't even check anything above about 12 in the tests because
of computation time -- I'm a bit wary of incorrectly entering many
(hundreds of) values that won't be tested. What do you think about just
clearly referencing that paper in the docstring for sequences longer than
32 bits?

Member

WarrenWeckesser commented Apr 26, 2014

Or if the state is all zeros, we could just return zeros immediately with a warning. Up to you.

Either an error or a warning seems reasonable. I'll kick the ball back and leave it up to you.

I'd rather stick with the 32 that are in there. [...] What do you think about just clearly referencing that paper in the docstring for sequences longer than 32 bits?

Sounds good. This is another case where it will be trivial to increase the maximum in the future if there is interest.

Member

WarrenWeckesser commented Apr 26, 2014

It occurs to me that something like len, length, or size might be preferable to nsamples. Just a thought...

Member

larsoner commented Apr 26, 2014

I like length. Do you know of any precedence elsewhere in scipy?

Member

WarrenWeckesser commented Apr 26, 2014

length works for me. On the other hand, the numpy and scipy random variable samplers (e.g. numpy.random.normal, scipy.stats.norm.rvs) use size, which can be an integer or a tuple
of integers. I'm not sure how compelling that comparison is; length (just an integer) feel more appropriate in this case.

Member

josef-pkt commented Apr 26, 2014

Why are you using "random" terminology. Is there anything random?
I didn't look long enough to know what this is supposed to do.

Member

WarrenWeckesser commented Apr 27, 2014

@josef-pkt: I was looking for functions that generate data to see what they call the argument that specifies how much data to generate. In my version of the MLS function, I have an argument called nsamples, but, as I suggested in a previous comment, something like length or size might be a better name, and I was wondering what else has been used in scipy functions. The functions that generate random samples came to mind.

MLS is deterministic (but it is a pseudo-random binary sequence).

Member

josef-pkt commented Apr 27, 2014

I was looking at the windows functions which just have an ugly N or similar.

My main problem would be with seed if it is not a seed to initialize a (pseudo) random number generator. I would find init or start or similar better if it starts a sequence. (lfilter uses zi)
I think for the number of samples length is better if it is the length of a sequence, size in random indicates the number of pseudo-"independent" draws/samples.

(comments from the outside)

Member

WarrenWeckesser commented Apr 27, 2014

Re seed: Eric called it seeds in the PR (but I don't see the need for it to be plural); I called it state in the first version of my function, but decided seed was fine, especially after reading "In mathematical literature, the initial state of either form is called the seed." at http://www.newwaveinstruments.com/resources/articles/m_sequence_linear_feedback_shift_register_lfsr.htm
I'm OK with any of seed, state, init, etc.

I think for the number of samples length is better if it is the length of a sequence, ...

Yes, I agree.

Coverage Status

Coverage remained the same when pulling b8bc1ad on Eric89GXL:add-mls into f129332 on scipy:master.

Coverage Status

Coverage remained the same when pulling b8bc1ad on Eric89GXL:add-mls into f129332 on scipy:master.

Member

larsoner commented Apr 28, 2014

Since a couple people have tripped over it, I change seeds to state to hopefully avoid confusion. @WarrenWeckesser I think this should address your comments, Travis is happy, and tests pass on my machine. Let me know what you think.

Member

WarrenWeckesser commented Apr 30, 2014

Eric: Just letting you know that it might be a few days before I get back to this.

Member

larsoner commented Apr 30, 2014

No problem -- thanks for letting me know!

@WarrenWeckesser WarrenWeckesser commented on an outdated diff May 3, 2014

scipy/signal/_max_len_seq.pyx
@@ -0,0 +1,119 @@
+# Author: Eric Larson
+# 2014
+
+"""Tools for MLS generation"""
+
+import numpy as np
+cimport numpy as np
+cimport cython
+
+__all__ = ['max_len_seq']
+
+
+# These are definitions of linear shift register taps for use in mls()
+_mls_taps = {2: [1], 3: [2], 4: [3], 5: [2], 6: [5], 7: [6], 8: [7, 6, 1],
@WarrenWeckesser

WarrenWeckesser May 3, 2014

Member

At the web site that you reference, the first set of taps listed for the 5 stage LFSR (http://www.newwaveinstruments.com/resources/articles/m_sequence_linear_feedback_shift_register_lfsr/5stages.txt) is [5, 3], so the _mls_taps[5] should be [3]. (Both sets of taps are valid; one sequence is a shifted and reversed version of the other.)

@WarrenWeckesser WarrenWeckesser commented on an outdated diff May 3, 2014

scipy/signal/_max_len_seq.pyx
+ 31: [28], 32: [31, 30, 10]}
+
+@cython.cdivision(True) # faster modulo
+@cython.boundscheck(False) # designed to stay within bounds
+@cython.wraparound(False) # we don't use negative indexing
+def max_len_seq(nbits, state=None, length=None, taps=None):
+ """
+ max_len_seq(nbits, state, length, taps)
+
+ Maximum Length Sequence (MLS) generator
+
+ Parameters
+ ----------
+ nbits : int
+ Number of bits to use. Length of the resulting sequence will
+ be ``(2**n) - 1``. Note that generating long sequences (e.g.,
@WarrenWeckesser

WarrenWeckesser May 3, 2014

Member

The argument is nbits, so use nbits in the formula.

@WarrenWeckesser WarrenWeckesser commented on an outdated diff May 3, 2014

scipy/signal/_max_len_seq.pyx
+@cython.cdivision(True) # faster modulo
+@cython.boundscheck(False) # designed to stay within bounds
+@cython.wraparound(False) # we don't use negative indexing
+def max_len_seq(nbits, state=None, length=None, taps=None):
+ """
+ max_len_seq(nbits, state, length, taps)
+
+ Maximum Length Sequence (MLS) generator
+
+ Parameters
+ ----------
+ nbits : int
+ Number of bits to use. Length of the resulting sequence will
+ be ``(2**n) - 1``. Note that generating long sequences (e.g.,
+ greater than ``nbits == 16``) can take a long time.
+ state : array | None, optional
@WarrenWeckesser

WarrenWeckesser May 3, 2014

Member

The type for both state and taps should be array_like. This lets the user know that any sequence is allowed, not just numpy arrays.

@WarrenWeckesser WarrenWeckesser commented on an outdated diff May 3, 2014

scipy/signal/_max_len_seq.pyx
+ Returns
+ -------
+ seq : array
+ Resulting MLS sequence of -1's and 1's.
+ state : array
+ The final state of the shift register.
+
+ Notes
+ -----
+ Algorithm for MLS generation generically described in:
+
+ http://en.wikipedia.org/wiki/Maximum_length_sequence
+
+ Values for taps specifically taken from:
+
+ http://www.newwaveinstruments.com/resources/articles/m_sequence_linear_feedback_shift_register_lfsr.htm # noqa, analysis:ignore
@WarrenWeckesser

WarrenWeckesser May 3, 2014

Member

This line is too long. Perhaps split it at the end of ...articles/.

Member

WarrenWeckesser commented May 3, 2014

There is a test for a bad taps argument, but that is the only test using taps. Tests of the valid uses of taps are still needed. Currently taps is not working. For example,

In [23]: seq, state = max_len_seq(5, taps=[2])
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-23-1ffff85e7737> in <module>()
----> 1 seq, state = max_len_seq(5, taps=[2])

/Users/warren/local_scipy/lib/python2.7/site-packages/scipy/signal/_max_len_seq.so in scipy.signal._max_len_seq.max_len_seq (scipy/signal/_max_len_seq.c:2079)()

ValueError: ndarray is not C-contiguous

@WarrenWeckesser WarrenWeckesser commented on an outdated diff May 3, 2014

scipy/signal/_max_len_seq.pyx
+ Algorithm for MLS generation generically described in:
+
+ http://en.wikipedia.org/wiki/Maximum_length_sequence
+
+ Values for taps specifically taken from:
+
+ http://www.newwaveinstruments.com/resources/articles/m_sequence_linear_feedback_shift_register_lfsr.htm # noqa, analysis:ignore
+ """
+ if taps is None:
+ if nbits not in _mls_taps:
+ known_taps = np.array(list(_mls_taps.keys()))
+ raise ValueError('Taps must be between %s and %s'
+ % (known_taps.min(), known_taps.max()))
+ taps = np.array(_mls_taps[nbits], int)
+ else:
+ taps = np.flipud(np.unique(np.array(taps, int)))
@WarrenWeckesser

WarrenWeckesser May 3, 2014

Member

Minor quibble: using flipud on a 1-D array is overkill. taps = np.unique(np.array(taps, int))[::-1] is simpler.

Coverage Status

Coverage remained the same when pulling e007fef on Eric89GXL:add-mls into f129332 on scipy:master.

Member

larsoner commented May 5, 2014

@WarrenWeckesser thanks for the thorough feedback. Comments have been addressed: adding tap-specifying tests, fixing taps use, removing np.flipud, splitting a long line, updating docstring, and adjusting the default taps for one case. Tests pass over here. Travis appears to be happy, except that one of the test suites took too long, which has happened on about half of my commits even though these tests are pretty quick.

Coverage Status

Coverage remained the same when pulling 04760e3 on Eric89GXL:add-mls into f129332 on scipy:master.

@WarrenWeckesser WarrenWeckesser commented on an outdated diff May 13, 2014

scipy/signal/_max_len_seq.pyx
+
+# These are definitions of linear shift register taps for use in max_len_seq()
+_mls_taps = {2: [1], 3: [2], 4: [3], 5: [3], 6: [5], 7: [6], 8: [7, 6, 1],
+ 9: [5], 10: [7], 11: [9], 12: [11, 10, 4], 13: [12, 11, 8],
+ 14: [13, 12, 2], 15: [14], 16: [15, 13, 4], 17: [14],
+ 18: [11], 19: [18, 17, 14], 20: [17], 21: [19], 22: [21],
+ 23: [18], 24: [23, 22, 17], 25: [22], 26: [25, 24, 20],
+ 27: [26, 25, 22], 28: [25], 29: [27], 30: [29, 28, 7],
+ 31: [28], 32: [31, 30, 10]}
+
+@cython.cdivision(True) # faster modulo
+@cython.boundscheck(False) # designed to stay within bounds
+@cython.wraparound(False) # we don't use negative indexing
+def max_len_seq(nbits, state=None, length=None, taps=None):
+ """
+ max_len_seq(nbits, state, length, taps)
@WarrenWeckesser

WarrenWeckesser May 13, 2014

Member

Include the default value (None) in the signature here.

@WarrenWeckesser WarrenWeckesser commented on an outdated diff May 13, 2014

scipy/signal/_max_len_seq.pyx
+@cython.cdivision(True) # faster modulo
+@cython.boundscheck(False) # designed to stay within bounds
+@cython.wraparound(False) # we don't use negative indexing
+def max_len_seq(nbits, state=None, length=None, taps=None):
+ """
+ max_len_seq(nbits, state, length, taps)
+
+ Maximum Length Sequence (MLS) generator
+
+ Parameters
+ ----------
+ nbits : int
+ Number of bits to use. Length of the resulting sequence will
+ be ``(2**nbits) - 1``. Note that generating long sequences
+ (e.g., greater than ``nbits == 16``) can take a long time.
+ state : array-like | None, optional
@WarrenWeckesser

WarrenWeckesser May 13, 2014

Member

The convention in numpy and scipy is to use array_like. Also, there is no need to include | None. Just array_like, optional is sufficient. (Same comment for the other parameters.)

@WarrenWeckesser WarrenWeckesser commented on an outdated diff May 13, 2014

scipy/signal/_max_len_seq.pyx
+
+ Notes
+ -----
+ Algorithm for MLS generation generically described in:
+
+ http://en.wikipedia.org/wiki/Maximum_length_sequence
+
+ Values for taps specifically taken from:
+
+ http://www.newwaveinstruments.com/resources/articles/
+ m_sequence_linear_feedback_shift_register_lfsr.htm
+ """
+ if taps is None:
+ if nbits not in _mls_taps:
+ known_taps = np.array(list(_mls_taps.keys()))
+ raise ValueError('Taps must be between %s and %s'
@WarrenWeckesser

WarrenWeckesser May 13, 2014

Member

nbits must be between...

Member

WarrenWeckesser commented May 13, 2014

Eric, I made a few comments in the code about fixing a few minor things.

API question: I don't think there is a need to restrict length to be not greater than 2**nbits - 1. The code could just continue repeating the sequence. Then someone could generate, say, 1000 bits of PRBS7 without having to repeatedly call max_len_seq. What do you think?

Also, I don't think this needs to be added to the waveforms module. I'd be fine with only exposing it as scipy.signal.max_len_seq. Most of the scipy subpackages maintain very flat namespaces. We've had discussions on the mailing list about this in the past, and while we're not actively deprecating older submodules, for new code we tend to create "private" modules, and only expose the new functions or objects at the package level. waveforms is an older module; if it were added today, we'd probably call it _waveforms, with the underscore prefix indicating a private module.

Member

larsoner commented May 13, 2014

Thanks for the info -- most of the packages I work with use a more flat namespace / _whatever.py convention, so it was actually cool to learn the __all__ way of doing it. I still like the other way better, so I was happy to change it :)

Suggestions should be addressed. Let me know if I didn't incorporate _max_len_seq.pyx appropriately. I made a new test_max_len_seq.py file to hold the tests, since that seemed the cleanest way. I can put it back in test_waveforms.py if that's preferable. Also, I made a couple of small cosmits to PEP8-ify a few lines -- if you don't like them, just let me know and I'll revert. Thanks (again) for the thorough review.

Member

larsoner commented May 13, 2014

And to answer your question -- I don't mind permitting longer lengths. Using an excessively long length will certainly be slower than calculating the proper length exactly once and then tiling appropriately, but getting the logic correct for this is probably more work than it would be worth, so I've enabled longer lengths to be used.

Coverage Status

Coverage remained the same when pulling 1a58c8c on Eric89GXL:add-mls into f129332 on scipy:master.

@rgommers rgommers added this to the 0.15.0 milestone May 24, 2014

@WarrenWeckesser WarrenWeckesser commented on an outdated diff May 28, 2014

scipy/signal/_max_len_seq.pyx
+@cython.boundscheck(False) # designed to stay within bounds
+@cython.wraparound(False) # we don't use negative indexing
+def max_len_seq(nbits, state=None, length=None, taps=None):
+ """
+ max_len_seq(nbits, state=None, length=None, taps=None)
+
+ Maximum Length Sequence (MLS) generator
+
+ Parameters
+ ----------
+ nbits : int
+ Number of bits to use. Length of the resulting sequence will
+ be ``(2**nbits) - 1``. Note that generating long sequences
+ (e.g., greater than ``nbits == 16``) can take a long time.
+ state : array_like, optional
+ If array, must be of length nbits, and will be cast to binary
@WarrenWeckesser

WarrenWeckesser May 28, 2014

Member

Backticks around nbits.

@WarrenWeckesser WarrenWeckesser commented on an outdated diff May 28, 2014

scipy/signal/_max_len_seq.pyx
+ If array, must be of length nbits, and will be cast to binary
+ (bool) representation. If None, a seed of ones will be used,
+ producing a repeatable representation. If ``state`` is all
+ zeros, an error is raised as this is invalid. Default: None.
+ length : int | None, optional
+ Number of samples to compute. If None, the entire length
+ ``(2**nbits) - 1`` is computed.
+ taps : array_like, optional
+ Polynomial taps to use (e.g., ``[7, 6, 1]`` for an 8-bit sequence).
+ If None, taps will be automatically selected (for up to
+ ``nbits == 32``).
+
+ Returns
+ -------
+ seq : array
+ Resulting MLS sequence of -1's and 1's.
@WarrenWeckesser

WarrenWeckesser May 28, 2014

Member

...sequence of 0's and 1's.

@WarrenWeckesser WarrenWeckesser commented on an outdated diff May 28, 2014

scipy/signal/_max_len_seq.pyx
+ ``(2**nbits) - 1`` is computed.
+ taps : array_like, optional
+ Polynomial taps to use (e.g., ``[7, 6, 1]`` for an 8-bit sequence).
+ If None, taps will be automatically selected (for up to
+ ``nbits == 32``).
+
+ Returns
+ -------
+ seq : array
+ Resulting MLS sequence of -1's and 1's.
+ state : array
+ The final state of the shift register.
+
+ Notes
+ -----
+ Algorithm for MLS generation generically described in:
@WarrenWeckesser

WarrenWeckesser May 28, 2014

Member

Here and below, a complete sentence would be better. E.g. The algorithm for MLS generation is generically...

Member

WarrenWeckesser commented May 28, 2014

Other than the few minor comments I just made inline, I think this is ready to go.

Member

larsoner commented May 28, 2014

Awesome. Just pushed a commit to address the comments.

Owner

rgommers commented May 28, 2014

Tip for future commits: if you're only changing documentation, you can avoid triggering TravisCI by including [ci skip] in your commit message. Saves them an hour cpu time.

Member

larsoner commented May 28, 2014

Cool, didn't know about that one. I had just manually been stopping the builds on repos I had permissions for in those cases. Thanks.

Member

WarrenWeckesser commented May 28, 2014

Whoops, I should have said almost ready to go.

The release notes (https://github.com/scipy/scipy/blob/master/doc/release/0.15.0-notes.rst) need a short note about the new function.

Also, add .. versionadded:: 0.15.0 to the docstring. (@rgommers, what's the standard spot for that? When I started working on scipy, the convention seemed to be to put it in the notes (and that's my preference), but lately folks are adding it before the "Parameters" section.)

Finally, could you rebase and squash your commits? Just a few commits (or even one) are all that we need in the history.

Owner

rgommers commented May 28, 2014

+1 for in Notes section

Member

larsoner commented May 28, 2014

Commits squashed, versionadded added, and notes added to release notes. Let me know if there's anything else, and thanks again for the thorough reviews.

Coverage Status

Coverage increased (+0.0%) when pulling b68279d on Eric89GXL:add-mls into 57e8fb9 on scipy:master.

@WarrenWeckesser WarrenWeckesser added a commit that referenced this pull request May 28, 2014

@WarrenWeckesser WarrenWeckesser Merge pull request #3351 from Eric89GXL/add-mls
ENH: signal: Add MLS method
acc7c94

@WarrenWeckesser WarrenWeckesser merged commit acc7c94 into scipy:master May 28, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

Coverage Status

Coverage increased (+0.0%) when pulling b68279d on Eric89GXL:add-mls into 57e8fb9 on scipy:master.

Member

WarrenWeckesser commented May 28, 2014

Merged. Thanks Eric, for the code and for your patience. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment