urinieto / msaf

Music Structure Analysis Framework
MIT License
490 stars 78 forks source link

Feature Tonnetz can't be calculated properly #108

Closed jzq2000 closed 3 years ago

jzq2000 commented 3 years ago

Hi, I think these codes will cause bugs when calling 'self._framesync_features = self.compute_features()'

    def _compute_all_features(self):
        """Computes all the features (beatsync, framesync) from the audio."""
        # Read actual audio waveform
        self._audio, _ = librosa.load(self.file_struct.audio_file,
                                      sr=self.sr)

        # Get duration of audio file
        self.dur = len(self._audio) / float(self.sr)

        # Compute actual features
        self._framesync_features = self.compute_features()

        # Compute framesync times
        self._compute_framesync_times()

        # Compute/Read beats
        self._est_beats_times, self._est_beats_frames = self.estimate_beats()
        self._ann_beats_times, self._ann_beats_frames = self.read_ann_beats()

        # Beat-Synchronize
        pad = True  # Always append to the end of the features
        self._est_beatsync_features, self._est_beatsync_times = \
            self.compute_beat_sync_features(self._est_beats_frames,
                                            self._est_beats_times, pad)
        self._ann_beatsync_features, self._ann_beatsync_times = \
            self.compute_beat_sync_features(self._ann_beats_frames,
                                            self._ann_beats_times, pad)
class Tonnetz(Features):
    """This class contains the implementation of the Tonal Centroids.

    The Tonal Centroids (or Tonnetz) contain harmonic content of a given audio
    signal.
    """
    def __init__(self, file_struct, feat_type, sr=config.sample_rate,
                 hop_length=config.hop_size, n_bins=config.tonnetz.bins,
                 norm=config.tonnetz.norm, f_min=config.tonnetz.f_min,
                 n_octaves=config.tonnetz.n_octaves):
        """Constructor of the class.

        Parameters
        ----------
        file_struct: `msaf.input_output.FileStruct`
            Object containing the file paths from where to extract/read
            the features.
        feat_type: `FeatureTypes`
            Enum containing the type of features.
        sr: int > 0
            Sampling rate for the analysis.
        hop_length: int > 0
            Hop size in frames for the analysis.
        n_bins: int > 0
            Number of bins for the CQT computation.
        norm: int > 0
            Normalization parameter.
        f_min: float > 0
            Minimum frequency.
        n_octaves: int > 0
            Number of octaves.
        """
        # Init the parent
        super().__init__(file_struct=file_struct, sr=sr, hop_length=hop_length,
                         feat_type=feat_type)
        # Init the local parameters
        self.n_bins = n_bins
        self.norm = norm
        self.f_min = f_min
        self.n_octaves = n_octaves

    @classmethod
    def get_id(self):
        """Identifier of these features."""
        return "tonnetz"

    def compute_features(self):
        """Actual implementation of the features.

        Returns
        -------
        tonnetz: np.array(N, F)
            The features, each row representing a feature vector for a give
            time frame/beat.
        """
        pcp = PCP(self.file_struct, self.feat_type, self.sr, self.hop_length,
                  self.n_bins, self.norm, self.f_min, self.n_octaves).features
        tonnetz = librosa.feature.tonnetz(chroma=pcp.T).T
        return tonnetz

If Tonnetz .feat_type is FeatureTypes.est_beatsync, The feat_type of Pcp in compute_features() is also FeatureTypes.est_beatsync rather than FeatureTypes.framesync.

I wonder if this will work?

        # Compute actual features
        feat_type = self.feat_type
        self.feat_type = FeatureTypes.framesync
        self._framesync_features = self.compute_features()
        self.feat_type = feat_type
urinieto commented 3 years ago

I believe that compute_features will always return framesync features, regardless on your feat_type. So I don't think this might cause an error. If you want the synchronous features, you may use the compute_beat_sync_features function. Does this make sense?

jzq2000 commented 3 years ago

But here PCP feat_type will be FeatureTypes.est_beatsync, and therefore its shape is wrong (based on beat).

 pcp = PCP(self.file_struct, self.feat_type, self.sr, self.hop_length,
                  self.n_bins, self.norm, self.f_min, self.n_octaves).features  
urinieto commented 3 years ago

Oh, you're totally right, I forgot about the features getter! I believe your solution would work.

Would you mind creating a PR with this? Nice catch!