Fix some reference issues when creating a ScoreVariant

[leleogere]

Closes #450.

This PR adds a couple of attributes that were not correctly handled when a ScoreVariant would be produced, resulting in RecursionErrors (and even segmentation faults with some scores) when trying to pickle some unfolded scores:

import warnings
from pathlib import Path
import pickle
import sys
import partitura as pt
import partitura.score

sys.setrecursionlimit(2**31-1)

score_path = Path("~/Documents/datasets/asap-dataset/Liszt/Sonata/xml_score.musicxml").expanduser()

with warnings.catch_warnings():
    warnings.simplefilter("ignore")
    score = pt.load_musicxml(score_path)
    score = pt.score.unfold_part_maximal(score)
with open("/tmp/test.pkl", "wb") as f:
    pickle.dump(score, f)
# Segmentation fault (core dumped)

The added references are the following:

• GenericNote.fermata
• Note.beam
• UnpitchedNote.beam
• Beam.notes
• Fermata.ref EDIT: Seems problematic, see comment below

Before, those attributes were not updated during the creation of a ScoreVariant, resulting in them pointing to the elements in the original score. Now, the references are correctly updated to point to the corresponding elements in the created ScoreVariant.

[leleogere]

I have a question about the failing test. It's failing because I've added the attribute ref of Fermata to the _ref_attr list, believing this was supposed to point to either a Note or a Barline:

class Fermata(TimedObject):
    """A Fermata sign.

    Parameters
    ----------
    ref : :class:`TimedObject` or None, optional
        An object to which this fermata applies. In practice this is a
        Note or a Barline. Defaults to None.

    Attributes
    ----------
    ref : :class:`TimedObject` or None
        See parameters

    """

However, it seems that this attribute can also be a string: "left", "middle", "right" according to what I've seen in the codebase, which causes the test failure. Can this attribute be either a TimedObject or a string? If so, I'll probably have to tweak a bit the logic of replace_refs because it supposes that _ref_attrs contains TimedObjects (with start and end attributes). It might also be a good thing to update the Fermata doc to reflect that this attribute can also be a string.

It seems that @mgrachten worked on fermatas, could I get your point of view on this?

[mgrachten]

I'm not aware that Fermata.ref is intended to be a string. Indeed it seems the musicxml import/export code uses the attribute to store/expect a string, but I think this is incorrect. The import code should probably set Fermata.ref to the Barline object it just created.

partitura/partitura/io/importmusicxml.py

Lines 647 to 679 in 0c76286

	location = get_value_from_attribute(e, "location", str)
	if (location is None) or (location == "right"):
	position_barline = measure_maxtime
	elif location == "left":
	position_barline = measure_start
	else:
	position_barline = position
	repeat = e.find("repeat")
	if repeat is not None:
	_handle_repeat(repeat, position_barline, part, ongoing)
	ending = e.find("ending")
	if ending is not None:
	_handle_ending(ending, position_barline, part, ongoing)
	bar_style_e = e.find("bar-style")
	if bar_style_e is not None:
	bar_style = score.Barline(bar_style_e.text)
	part.add(bar_style, position)
	# <!ELEMENT barline (bar-style?, %editorial;, wavy-line?,
	# segno?, coda?, (fermata, fermata?)?, ending?, repeat?)>
	# <!ATTLIST barline
	# location (right | left | middle) "right"
	# segno CDATA #IMPLIED
	# coda CDATA #IMPLIED
	# divisions CDATA #IMPLIED
	fermata_e = e.find("fermata")
	if fermata_e is not None:
	location = e.get("location")
	fermata = score.Fermata(location)

Actually, I don't understand why it gets the location attribute a second time, when it's already defined above. It seems like repeated tweaks to this code over time made it somewhat inconsistent.

[leleogere]

I would argue that if this location is needed, it should be added to the Barline object (like in the MusicXML specification), and that the ref attribute of the fermata should indeed be either a Note or a Barline.

EDIT: If I'm understanding correctly this location attribute, I would say that it's not needed. The Barline should simply be placed in the partitura timeline according to it, but there is no need to store it as an attribute of the object.

[leleogere]

It seems like repeated tweaks to this code over time made it somewhat inconsistent.

It seems that there might be quite a few problems in that barline code indeed. A few things that I find strange are:

• the Barline is added on the timeline at position, shouldn't it be added at position_barline (taking into account the location attribute)?
• the Barline is only added if the bar-style attribute is not None, which mean that we wouldn't be able to set the ref attribute of a fermata on a a barline without bar-style.
• If the barline location is None, and it has a fermata, the fermata location position is assumed to be "right" according to the comment, and appended to the trailing_children list containing elements that should be added at the end of the measure. However, if the location is explicitly "right", the fermata is directly added at the current position in the timeline, rather than appended in trailing_children.

There might be other issues, but that's the first ones that came to my mind while reading that code.

[mgrachten]

All valid points. Since you have a good grasp on the issues, feel free to resolve as you see fit. I currently don't have the bandwidth to address this.

[github-actions]

This pull request had no activity for 6 months. It will be closed in 2 weeks unless there is some new activity.

[CarlosCancino-Chacon]

Hi @leleogere! Are there any updates on this PR?

[leleogere]

Hi! Unfortunately, I did not take to time to dig into that Barline issues. This PR should probably wait until I (or someone else) fix that, as merging it as is will fix some scores, but cause issues with others that were working so far.