-
Notifications
You must be signed in to change notification settings - Fork 206
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
Stateful processing #244
Stateful processing #244
Conversation
440ec31
to
1139eaf
Compare
1139eaf
to
dfe5756
Compare
dfe5756
to
d31ea5c
Compare
d31ea5c
to
370fb38
Compare
370fb38
to
00a9d65
Compare
56147ce
to
d918205
Compare
d918205
to
2903951
Compare
8d5cd01
to
372a18a
Compare
6e3a374
to
d93a5c8
Compare
madmom/ml/hmm.pyx
Outdated
@@ -394,6 +394,38 @@ class HiddenMarkovModel(object): | |||
raise ValueError('Initial distribution is not a probability ' | |||
'distribution.') | |||
self.initial_distribution = initial_distribution | |||
# attributes needed for stateful processing (i.e. forward_step()) | |||
self._prev = self.initial_distribution.copy() | |||
self._cur = np.zeros_like(self._prev) |
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.
Maybe just call reset()
instead?
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.
Also, we could use the variables here in forward_generator
, instead of allocating additional memory (can't comment there, though, because it didn't change).
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.
Yes, calling reset()
does the same.
IMHO, we should get rid of forward_generator
altogether in the long run, since it basically does the same as forward()
with reset=False
called with individual observations. What the current implementation of forward()
doesn't support is handling of block_size
other than 1. Thus I did not remove it for now.
EDIT: the block_size
only affects the computation of the densities given the observations. So maybe we could get rid of forward_generator
sooner than thought if we don't care about the additional CPU cycles if we compute the densities frame by frame, too. If we don't want to remove it completely (since it is not exactly the same as calling forward()
multiple times), we can at least replace the duplicated logic inside this method and call forward()
instead.
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.
Ok, I opted against calling reset()
, because I think it is more explicit to initialise variables in __init__()
and not in any other method.
madmom/ml/hmm.pyx
Outdated
self.__dict__.update(state) | ||
# add non-pickled attributes needed for stateful processing | ||
self._prev = self.initial_distribution.copy() | ||
self._cur = np.zeros_like(self._prev) |
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.
Same here, just call reset()
?
out_processor = [rnn, pp, writer] | ||
# Note: we need np.atleast_2d and np.transpose before the RNN, since | ||
# it expects the data in 2D (1D means framewise processing) | ||
out_processor = [np.atleast_2d, np.transpose, rnn, pp, writer] |
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.
Wouldn't it be better to have the SpectralOnsetProcessor
return the correct shape (n_frames, 1)?
Having to use np.atleast_2d
and np.transpose
every time I use the SpectralOnsetProcessor
seems a bit unintuitive (although I also see returning a 2d-array when the data is 1d also as a bit unintuitive - not sure which one to prefer).
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.
Yes and no. I thought about this as well, but opted for the way of least invasion to existing code.
As you state, returning a 2d-array is a bit unintuitive.
There's a TODO item in #185 which says: "rewrite all features as processors (mostly madmom.features.onsets.*
)". I think it is better to address this separately and not in this PR.
madmom/ml/hmm.pyx
Outdated
cdef double [::1] tm_probabilities = tm.log_probabilities | ||
cdef unsigned int num_states = tm.num_states | ||
cdef int num_states = tm.num_states |
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.
Why not unsigned int?
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.
Basically it shouldn't matter, at least for num_states
. I was hoping to get rid of the problems on windows (#178) as well. I can revert line 451, but the others should be uint32_t
since it is supposed to be safer for multiple platforms.
madmom/ml/hmm.pyx
Outdated
|
||
# observation model stuff | ||
om = self.observation_model | ||
cdef unsigned int num_observations = len(observations) | ||
cdef unsigned int [::1] om_pointers = om.pointers | ||
cdef int num_observations = len(observations) |
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.
Why not unsigned int?
madmom/ml/hmm.pyx
Outdated
@@ -440,7 +468,7 @@ class HiddenMarkovModel(object): | |||
num_states), | |||
dtype=np.uint32) | |||
# define counters etc. | |||
cdef unsigned int state, frame, prev_state, pointer | |||
cdef int state, frame, prev_state, pointer |
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.
Why not unsigned int? (I'm repeating myself... :) )
madmom/ml/hmm.pyx
Outdated
# keep track of the normalisation sum | ||
prob_sum = 0 | ||
# iterate over all states | ||
for state in range(num_states): | ||
# we need to reset the current state in case this method | ||
# gets called multiple times (i.e. framewise processing) | ||
fwd_cur[frame, state] = 0 |
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.
You don't need this, because you initialise fwd_cur
with np.zeros
when calling this function.
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.
Yes, this is a leftover from a previous code version. Back then I also initialised fwd_cur
of length 1 during initialisation and only re-allocated it if called with observations with length > 1.
madmom/ml/hmm.pyx
Outdated
dtype=np.float) | ||
cdef double[::1] fwd_prev = self._prev | ||
cdef double[:, ::1] fwd_cur = \ | ||
np.zeros((num_observations, num_states), dtype=np.float) |
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.
- Since
fwd_cur
does not only hold the current forward values, but all of them (shape(num_observations, num_states)
), I would suggest to keep the old name,fwd
. - I don't understand why you need
fwd_prev
, if all the values are stored infwd_cur
anyways. You can access the previous values withfwd_cur[frame - 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.
- ok, this makes sense.
fwd_cur
holds onlynum_observations
, notnum_observations + 1
variables any more. The previous variables are stored inself._prev
. This change was necessary (at least I think it is) because during initialisation we know how many states we have and can thus initself._prev
, but don't know the lengths of the observations yet. If we want to be able to callforward()
with observations of variable length (i.e. both a complete sequence or frame-by-frame), we have to initfwd_cur
(orfwd
, see 1.) insideforward()
but need to keep the previous variables somewhere, henceself._prev
.
needed to pass reset=False or other keywords to process() as proposed in #33 to distinguish between stateful and stateless processing (e.g. for HMMs or RNNs)
revert the separate computation/activation of complete sequences vs. stepwise processing; layers get resetted automatically when a whole sequence is processed
d93a5c8
to
5a9b3c7
Compare
This PR adds proper stateful processing.
The
process()
method of stateful processors accepts an additional/newreset
argument, which defaults toTrue
and thus does not change existing behaviour. This is implemented forHMMs
andRNNs
.To be able to process sequences either as a whole or on a frame-by-frame basis,
process()
expects always the same data dimensionality.NeuralNetwork
previously corrected the dimensions if a 1-dimensional input feature is expected (e.g.SuperFluxNN
). This conversion is moved to the executable program instead.If data is processed framewise,
reset=False
must be passed to the processors, otherwise stateful processors get reseted constantly. To be able to pass additional keyword arguments,madmom.processors
was adapted accordingly.