Skip to content

remove empty measure in svg#113

Merged
weixlu merged 1 commit into
liebharc:mainfrom
weixlu:svg
Jul 7, 2026
Merged

remove empty measure in svg#113
weixlu merged 1 commit into
liebharc:mainfrom
weixlu:svg

Conversation

@weixlu

@weixlu weixlu commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Describe the bug

When running convert_lieder.py, the following message can often be seen:

INFO: Number of measures in SVG files 25.0 does not match number of measures in XML 26

This happens because, when counting the number of measures in .svg files, empty measures cannot be correctly detected and skipped. For example, see the image below, this barline should be removed:
image

#102 partially fixed this issue, but the approach is not robust enough. For example, the barline below would also be incorrectly removed.

image

How to fix

This PR detects whether a measure contains any notes, and then decides whether to remove.

Specifically:

  1. In get_position_information_from_svg(), in addition to collecting barline, this PR adds _parse_note_position() to collect the positions of notes (including rests). It then calls the newly added _remove_empty_measures() to remove empty measures.

  2. The way empty measures are removed is as follows: first, staves are matched by system. Then, for each system, the newly added _staff_has_content_between() is called to check whether any note falls within any staff. If not, the corresponding barline is removed.

Tests

I mainly verified by checking the logs of convert_lieder.py, with particular attention to messages like:

Number of measures in SVG files xxx does not match number of measures in XML xxx

The results are:

  1. This message is significantly reduced.
  2. No new files fail because of this issue.
  3. A few files still report this mismatch, but after manually checking several of them, I found that the measure count from the SVG is correct, while the measure count from the XML is wrong.

(Finally the patch is done (😌relieved face), we do not need to merge this patch immediately. Later, I will train the model once with this patch and once without it, and compare the results.)

@liebharc

liebharc commented Jul 5, 2026

Copy link
Copy Markdown
Owner

Looks like you need to make the formatter happy (make format). After that changes look good. So feel free to merge it! And thanks.

@weixlu

weixlu commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

@liebharc Hi, I have finished a full training with and without the patch. Both models show SER at 6% on smoke test. Do you mind running a system level test? I'll send you the models via email later.

And now, since benchmark is available, I wonder if we also need to also do this before merging major changes.

@weixlu

weixlu commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

btw, unfortunately, I'm unable to get access to SMB dataset, although it's open sourced.

And last time I (and my colleagues here in China) try access Meta's Llama model, we get rejected too.😟

image

@liebharc

liebharc commented Jul 7, 2026

Copy link
Copy Markdown
Owner

btw, unfortunately, I'm unable to get access to SMB dataset, although it's open sourced.

Hi, we also had to email them so likely no reason to worry yet. As they say in their note: "As sometimes 🤗 is not emailing me when someone requests access. If you are interested, please reach out via email.". @aicelen , do you remember the email address you used?

@liebharc

liebharc commented Jul 7, 2026

Copy link
Copy Markdown
Owner

And now, since benchmark is available, I wonder if we also need to also do this before merging major changes.

Yes I'm wondering about that too. I started running polish scores with musicdiff, although that might not be perfect. It is dominated by beam/flag errors as homr simply not making that difference. So in general it would make sense to use polish scores as reference benchmark more in the future, but we would first need to figure out how. Perhaps we should stick to our own parsers as metric? That can cause issues if there is a code update, but a) that can also happen with musicdiff and b) we had the same issue with my testset in the past and the simple solution is to just make a note explaining that in Training.md.

I would likely still run my small test set as it's real world images taken by people. If we ever get enough support then we could create a dataset in which people print out reference music which isn't restricted by copy right and use that.

@weixlu

weixlu commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Hi, we also had to email them so likely no reason to worry yet.

Oh, I get it, then I'll email them

@weixlu

weixlu commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

I'd suggest using our own parsers, instead of musicdiff. I think this depends on my goal:

  1. Here my goal is to ensure the new patch doesn't make things worse. So I'd ignore the beam/flag errors, just focus on what homr supports right now.
  2. If my goal is to learn from other OMR tools or parsers, then maybe I shouldn't use our own parser, so avoid the bias in homr's favour

@weixlu

weixlu commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

That can cause issues if there is a code update

Sure, code and dataset changes overtime, making it hard to compare the current result with the one earlier. I personally think that's OK (or acceptable). It's very hard to keep backward compatibility‌.

@aicelen

aicelen commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

This is the mail address I wrote to: jcmartinez.sevilla@ua.es
I found it on their own website so it should be ok to share it here

@weixlu

weixlu commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

This is the mail address I wrote to: jcmartinez.sevilla@ua.es I found it on their own website so it should be ok to share it here

OK, Thanks!

@weixlu

weixlu commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Test result looks good, so I'll merge it. Thanks @liebharc for helping with validation!

@weixlu
weixlu merged commit 79aec9b into liebharc:main Jul 7, 2026
1 check passed
@aicelen

aicelen commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

How does the newly trained model compare to model 396? If it is better, could you upload it (and the pytorch weights)?

@weixlu

weixlu commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

How does the newly trained model compare to model 396?

They performed nealy the same

  • this: 5.9 diffs, SER: 4.9%
  • 396: 6.7 diffs, SER: 5.2%

If it is better, could you upload it (and the pytorch weights)

Yes, of course. But I havn't figured out how to upload model. I'll look into it tomorrow

@liebharc

liebharc commented Jul 10, 2026

Copy link
Copy Markdown
Owner

Hi @weixlu , just wanted to check: Did you hear ever back regarding your SMB access? I rarely use the dataset myself, so it might not be crucial, but still wanted to know how things went.

@weixlu

weixlu commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Hi @weixlu , just wanted to check: Did you hear ever back regarding your SMB access? I rarely use the dataset myself, so it might not be crucial, but still wanted to know how things went.

I have got the access! In fact, I run the benchmark and see it takes quite some time, So I plan to do some optimization next week.

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.

3 participants