Skip to content
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

Feature/evaluation #146

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Conversation

rakuri255
Copy link
Owner

No description provided.


class PitcherTest(unittest.TestCase):
# @pytest.mark.skip(reason="Skipping this FUNCTION level test, can be used for manual tests")
def test_get_pitch_with_crepe_file(self):
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test can be removed. PitcherTest class is in test_pitcher.py

import pytest
from src.modules.plot import plot


class PitcherTest(unittest.TestCase):
@pytest.mark.skip(reason="Skipping this FUNCTION level test, can be used for manual tests")
# @pytest.mark.skip(reason="Skipping this FUNCTION level test, can be used for manual tests")
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented skip

@@ -1,6 +1,7 @@
MIT License

Copyright (c) 2023 Vadim Rangnau
Copyright (c) 2020 Max Morrison (torchcrepe code adapted for crepe output filtering abd thresholding)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe fork crepe an make a separate package for it? Or PR the changes to the crepe project?

from src.modules.plot import plot


class PitcherTest(unittest.TestCase):
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also in test_pitcher.py

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

@@ -1,6 +1,7 @@
MIT License

Copyright (c) 2023 Vadim Rangnau
Copyright (c) 2020 Max Morrison (torchcrepe code adapted for crepe output filtering abd thresholding)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not fork crepe, move the changes there and release an package?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure how to give proper credit here. I would say it's up to you how to handle this and if you want to accept the code in question.

The code here https://github.com/rakuri255/UltraSinger/pull/146/files#diff-7bda13ea2689179c7952b68174b8b8ea2cc2250f9b9699c5cf55939fb6c4ac7d is copied and adapted from here https://github.com/maxrmorrison/torchcrepe/blob/master/torchcrepe/loudness.py

@@ -881,6 +1024,19 @@ def denoise_vocal_audio(input_path: str, output_path: str) -> None:
"""Denoise vocal audio"""
ffmpeg_reduce_noise(input_path, output_path)

# Fixme: Merge issue
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixme

@@ -8,6 +8,11 @@ def blue_highlighted(text: str) -> str:
return f"{Bcolors.blue}{text}{Bcolors.endc}"


def green_highlighted(text: str) -> str:
"""Returns a blue highlighted text"""
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commend says blue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

FILE_ENCODING,
)

CHARACTERS_TO_REMOVE = ["\ufeff"]
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this "BOM"? Than we should name it "BOM"

ultrastar_class = UltrastarTxtValue()
count = 0

# Strips the newline character
for line in txt:
filtered_line = line
for character_to_remove in CHARACTERS_TO_REMOVE:
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove in each line? Isnt this just an encoding issue?


@dataclass_json
@dataclass
class TranscriptionResult:
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just Transcription with data and detected_language?

# Conflicts:
#	src/UltraSinger.py
#	src/modules/Speech_Recognition/TranscribedData.py
#	src/modules/plot.py
start_time = (
beat_to_second(int(ultrastar_class.startBeat[pos]), real_bpm) + gap
)
gap = int(float(ultrastar_class.gap.replace(",", ".")) / 1000)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the score calculation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

real_bpm = ultrastar_bpm_to_real_bpm(
float(ultrastar_class.bpm.replace(",", "."))
)
gap = int(float(ultrastar_class.gap.replace(",", ".")) / 1000)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the score calculation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch.
another contributing factor to broken score calculation was the wrong usage of the constructor here:
76a4956#diff-f8287eea965b098ba5cef0e0b678bd57ea9d41c3985c3a311c940792de15c57fR987

Calling the constructor like this

new_transcribed_data.append(TranscribedData({"word": midi_segment.word, "start": midi_segment.start, "end": midi_segment.end, "is_hyphen": None, "confidence": 1}))

just initializes the confidence field to have the map as it's value and all other fields will have their default value. So all start and end values are 0 from then on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants