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

[WIP] namespace conversion #105

Merged
merged 4 commits into from Feb 17, 2016
Merged

[WIP] namespace conversion #105

merged 4 commits into from Feb 17, 2016

Conversation

bmcfee
Copy link
Contributor

@bmcfee bmcfee commented Feb 16, 2016

Implements #28

  • beat_position -> beat
  • tag_* -> tag_open
  • segment_* -> segment_label_open
  • pitch_hz <-> pitch_midi
  • chord_harte -> chord
  • ~~~pitch_class -> pitch_midi, pitch_hz~~~
  • ~~~chord_roman -> chord_harte (?)~~~

@bmcfee bmcfee self-assigned this Feb 16, 2016
@bmcfee bmcfee added this to the 0.3.0 milestone Feb 16, 2016
@bmcfee
Copy link
Contributor Author

bmcfee commented Feb 16, 2016

@justinsalamon give it a look. I'll whip up some tests shortly, but all you have to do now is say

>>> jams.convert(annotation, 'pitch_hz')

if such a conversion is possible, it will be done. If not, you get an exception.

It's designed to make adding new converters pretty easy (see the nsconvert.py module).

@justinsalamon
Copy link
Contributor

@bmcfee just had a look (hence ninja star), looks good to me.

@bmcfee
Copy link
Contributor Author

bmcfee commented Feb 16, 2016

Okay, cool. I'll get on with testing then.

I'll also rework eval a bit to use the converters where appropriate.

@justinsalamon
Copy link
Contributor

Sounds good, ping me if you'd like me to have a second look, and hopefully we can merge this pretty soon.

added pitch_hz to midi

added tag and segment openers

added beat_position to beat

added chord_harte to chord

added namespace conversion tests

fixed a bug in conversion no-op detection
@bmcfee
Copy link
Contributor Author

bmcfee commented Feb 16, 2016

Ok, I think this one's ready to go.

Give it a last CR pass and I'll merge.

raise NamespaceError('Expected namespace="{:s}", found "{:s}"'
.format(namespace, ann.namespace))

ann = convert(ann, namespace)
Copy link
Contributor

Choose a reason for hiding this comment

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

@bmcfee am I right in understanding that you're validating the jams by converting it to the desired namespace and then checking if it validates against this namespace?

If so, doesn't this mean you're not only doing validation, but also conversion, under the hood?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test was previously

"Is this annotation of namespace X?"

and is now

"can this annotation be interpreted as a valid instance of namespace X".

I think for evaluation (and sonification) the latter is what we really want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. So let's say I have a pitch_midi annotation, and I validate it against the pitch_hz namespace, which would validate fine in this new scenario. So I then try to use this annotation with the transcription eval wrapper, which expects pitch_hz annotations, and it breaks.

Now of course in practice I'll be adding a conversion step to the transcription eval wrapper such that any annotation provided is converted into pitch_hz, so this wouldn't actually happen. But could this type of scenario, where you validate against a namespace different to the one your annotation actually is (but convertible to) and consequently get errors further down the line somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I then try to use this annotation with the transcription eval wrapper, which expects pitch_hz annotations, and it breaks.

Ah, I see what you're saying. I forgot to change the return type.

Perhaps this should be renamed to coerce_annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've updated the validation logic.

Instead of returning true (which was never actually checked) it now returns the coerced annotation, which is what gets piped into mir_eval.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I just had a look. Already added a couple of line comments for your attention.

@bmcfee
Copy link
Contributor Author

bmcfee commented Feb 16, 2016

for example here, does this mean you would validate a chord_harte annotation by trying to convert it to the chord namespace and check if it validates there? Is this safe?

This is safe because chord_harte is a (proper) subset of chord. Going the other direction would not be safe in general. (Similarly for conversion between closed tag or segment vocabularies.)

It wouldn't be hard to do a best-effort reverse conversion (chord -> chord_harte) by clobbering the namespace and calling validate. If it passes, we're good. If it fails, reraise the exception saying that you have an uncastable input (eg, a sus chord).

I'd prefer to not go down that rabbit hole though, since it opens up a lot of possibilities for attempted conversion. Better to stick with a set of well-defined converters that can keep guarantees about the output.

ann.validate(strict=True)

return True
return ann
Copy link
Contributor

Choose a reason for hiding this comment

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

This look safer, yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

!resolved

bmcfee added a commit that referenced this pull request Feb 17, 2016
@bmcfee bmcfee merged commit 57e5e45 into master Feb 17, 2016
@bmcfee bmcfee modified the milestones: 0.2.2, 0.3.0 Feb 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants