gregchapman-dev / musicdiff

A music score notation diff package (and command line tool)
Other
32 stars 4 forks source link

musicdiff can crash in visualization, checking pitch on an Unpitched note. #11

Closed gregchapman-dev closed 8 months ago

gregchapman-dev commented 10 months ago

This was mentioned by Philippe Rigaux. I have since reproduced it.

xavriley commented 8 months ago

I also ran into this. The patch below fixed it for me but is somewhat of a hack:

diff --git a/musicdiff/comparison.py b/musicdiff/comparison.py
index 163d449..9c37c73 100644
--- a/musicdiff/comparison.py
+++ b/musicdiff/comparison.py
@@ -350,7 +350,7 @@ class Comparison:
             else:  # they are two notes
                 op_list.append(("pitchnameedit", noteNode1, noteNode2, 1, ids))
         # add for the accidentals
-        if pitch1[1] != pitch2[1]:  # if the accidental is different
+        if pitch1[1] != pitch2[1] and not (pitch1[0][0] == "R" or pitch2[0][0] == "R"):  # if neither is a rest and the accidentals are different
             cost += 1
             if pitch1[1] == "None":
                 assert pitch2[1] != "None"

Arguably it would be better to prevent the rests getting to that point but I didn't manage to trace it back further.

gregchapman-dev commented 8 months ago

Ah, good catch. I have fixed Philippe's crash in the develop branch, but not this particular instance. I have a bunch of improvements about to land in develop (and ultimately in a new release), so I'll make sure to fix this one as well.

gregchapman-dev commented 8 months ago

Here's my proposed fix (almost the same as yours, but I also skip the tie check for rests). Please try this and see if it works for you.

diff --git a/musicdiff/comparison.py b/musicdiff/comparison.py
index 612b04a..6ef60ec 100644
--- a/musicdiff/comparison.py
+++ b/musicdiff/comparison.py
@@ -349,6 +349,11 @@ class Comparison:
                 op_list.append(("pitchtypeedit", noteNode1, noteNode2, 1, ids))
             else:  # they are two notes
                 op_list.append(("pitchnameedit", noteNode1, noteNode2, 1, ids))
+
+        # accidental and tie comparisons don't make sense for rests
+        if pitch1[0] == 'R' or pitch2[0] == 'R':
+            return op_list, cost
+
         # add for the accidentals
         if pitch1[1] != pitch2[1]:  # if the accidental is different
             cost += 1
gregchapman-dev commented 8 months ago

Fixed in develop branch (both @rigaux's visualization.py report and @xavriley's comparison.py report) in commit https://github.com/gregchapman-dev/musicdiff/commit/ce262bdfd026cf19513d40546b07dd320e8b46f0

gregchapman-dev commented 8 months ago

Actually, I want to back out the comparison.py fix. Rests are annotated with accidental == "None" and tie == False, which can be compared just fine with other rests or notes. With the change I made, the cost ends up being less than expected when comparing a rest with a note, and you get some weird edit list decisions. I'd rather leave this alone.

@xavriley, was it causing a real problem? Or did you just not expect to see an accidental comparison on a rest?

xavriley commented 8 months ago

Thanks for the quick follow up. It was a problem in that it was raising an exception when trying to diff two scores:

Annotation type pitchtypeedit not yet supported for visualization
Traceback (most recent call last):
  File "/home/xavriley/.pyenv/versions/3.8.7/lib/python3.8/runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/home/xavriley/.pyenv/versions/3.8.7/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/home/xavriley/.pyenv/versions/3.8.7/envs/pytorch/lib/python3.8/site-packages/musicdiff/__main__.py", line 61, in <module>
    numDiffs: int = diff(args.file1, args.file2, detail=detail)
  File "/home/xavriley/.pyenv/versions/3.8.7/envs/pytorch/lib/python3.8/site-packages/musicdiff/__init__.py", line 160, in diff
    Visualization.mark_diffs(score1, score2, diff_list)
  File "/home/xavriley/.pyenv/versions/3.8.7/envs/pytorch/lib/python3.8/site-packages/musicdiff/visualization.py", line 634, in mark_diffs
    if note2.pitch.accidental:
AttributeError: 'Rest' object has no attribute 'pitch'

Here are the scores I used - they're arguably too long and have too many differences to be reasonably diff'ed, but maybe they can help with making a test case:

scores_for_comparison.zip

gregchapman-dev commented 8 months ago

Thank you for the scores! I'm always looking for good test data.

The crash you mentioned is fixed in develop, even though I have backed out the comparison.py change. Please try it out.