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

Regression issue - unable get fingering of cords after parsing xml file #1473

Closed
RoyShmuli opened this issue Oct 27, 2022 · 10 comments · Fixed by #1475
Closed

Regression issue - unable get fingering of cords after parsing xml file #1473

RoyShmuli opened this issue Oct 27, 2022 · 10 comments · Fixed by #1475

Comments

@RoyShmuli
Copy link

RoyShmuli commented Oct 27, 2022

music21 version

Issue on version 8.1.0
Working well on version 7.3.3

Problem summary

Regression issue unless there is a new way, I didn't find out.
Unable get all fingering of chord, I'm receiving only the first finger

image

Steps to reproduce

Use the following code and the attached file (simple cord)

for generalNote in music21.converter.parse(f"myFile.xml").parts[0].flat.notes:
  fingers = []

  # On version 8.1.0 print "generalNote.articulations.size: 1"
  # On version 7.3.3 print "generalNote.articulations.size: 3"
  print(f"generalNote.articulations.size: {len(generalNote.articulations)}")

  for articulation in (generalNote.articulations):
    if isinstance(articulation, music21.articulations.Fingering):
      fingers.append(articulation.fingerNumber)

  # fingers contain 1 finger on verion 8.1.1 and 3 fingers on version 7.3.3

Expected vs. actual behavior

Actual getting 1 finger
Expected getting 3 fingers

More information

@mscuthbert
Copy link
Member

I believe that this is a known change and improvement. If a note is a Chord, then you should iterate over it:

for chordNote in foundChord.notes:
     print(chordNote.articulations)

The reason for the change is that it is possible to associate a particular fingering with a particular note in a chord, not just three fingerings to the chord as a whole.

@RoyShmuli
Copy link
Author

RoyShmuli commented Oct 27, 2022

Thanks for quick response
The change make sense, but I'm getting 3 times empty list using your code.

Is there a doc of upgrade from version 7 to 8?

@mscuthbert
Copy link
Member

That'd be https://github.com/cuthbertLab/music21/releases/tag/v8.1.0

But I'm not seeing this there. I'll need to look at it over the weekend when I've recovered from an illness unless someone else remembers exactly what the issue is. I'm not sure my syntax above was exact, only that I remember the change being consciously made.

We're trying to have a better changelog for v9.

@RoyShmuli
Copy link
Author

Feel well 🙏

I think you right with the syntax

@mscuthbert
Copy link
Member

Am trying to replicate, but now seeing that there was no attached file. Can you attach?

@mscuthbert
Copy link
Member

hmmm, the relevant changes seem to have happened in 2018, that's why they're so vague in my memory: #323

@RoyShmuli
Copy link
Author

RoyShmuli commented Oct 28, 2022

The file is a simple chord, adding it here

myFile.zip

I think the issue on clear the articulations of the notes, in xmlToM21.py -> function xmlToChord
code:
n.articulations = [] # Removing this line resolve this issue
Maybe have the same issue with
n.expressions = []

I don't know if we can just remove them or there is a reason they exist

@RoyShmuli
Copy link
Author

Moreover (another related issue) I think in xmlToChord we copy the first finger to chord object, that's why I received one finger

for generalNote in music21.converter.parse(f"myFile.xml").parts[0].flat.notes:
  fingers = []

  # On version 8.1.0 print "generalNote.articulations.size: 1" for chord object
  print(f"generalNote.articulations.size: {len(generalNote.articulations)}")

@jacobtylerwalls
Copy link
Member

I bisected the change to b624ef3's changing pass to continue to match the way the surrounding code was documented. It improved some use cases but removed the support for importing multiple fingerings on one chord.

@mscuthbert
Copy link
Member

Thanks @RoyShmuli and @jacobtylerwalls -- I completely misdiagnosed the issue. It turns out we never did the movement of fingerings to notes also.

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 a pull request may close this issue.

3 participants