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

AB-91: Map model classes to more meaningful labels #348

Merged
merged 3 commits into from Jul 1, 2019

Conversation

@alastair
Copy link
Contributor

commented Jun 10, 2019

A continuation of #330 with a few additional changes to improve the code flow and number of queries that we make.
Also add some more mappings and improve the way that we display this data on the summary page.

Deployment process

Run sql update script admin/updates/20190606-highlevel-model-mappings.sql


UPDATE model SET class_mapping = '{
"acoustic": "Acoustic",
"not_acoustic": "Not acoustic"

This comment has been minimized.

Copy link
@alastair

alastair Jun 10, 2019

Author Contributor

I also updated these items so that we don't have to automatically remove _ from them.

}'::jsonb WHERE model.model = 'mood_sad';

UPDATE model SET class_mapping = '{
"Cluster1": "passionate, rousing, confident, boisterous, rowdy",

This comment has been minimized.

Copy link
@alastair

alastair Jun 10, 2019

Author Contributor

These cluster descriptions are listed here: https://acousticbrainz.org/datasets/accuracy#moods_mirex

@@ -265,17 +265,15 @@ def set_model_status(model_name, model_version, model_status):
"model_status": model_status})


def get_model(model_name, model_version):

This comment has been minimized.

Copy link
@alastair

alastair Jun 10, 2019

Author Contributor

this method wasn't used anywhere, so I hijacked the implementation to make a method that we needed


new_all = {}
for cl, val in highlevel["all"].items():
new_all[mapping[cl]] = val

This comment has been minimized.

Copy link
@alastair

alastair Jun 10, 2019

Author Contributor

this assumes that the mapping has all possible values in it. Since we control the both the models and the mapping definition I didn't bother checking it here

This comment has been minimized.

Copy link
@paramsingh

paramsingh Jun 24, 2019

Member

Sounds ok to me.


def interpret(text, data, threshold=.6):
if data['probability'] >= threshold:

This comment has been minimized.

Copy link
@alastair

alastair Jun 10, 2019

Author Contributor

I removed this threshold because it didn't make much sense with models that had many classes - e.g. with 6 classes, 40% is much higher than the random baseline (16%)

@alastair

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@paramsingh an eye over this would be appreciated, thanks

@@ -265,17 +265,15 @@ def set_model_status(model_name, model_version, model_status):
"model_status": model_status})


def get_model(model_name, model_version):
def get_active_models():

This comment has been minimized.

Copy link
@paramsingh

paramsingh Jun 24, 2019

Member

Should we just make this get_models(active=True), and get non active models too, for some future usage? I say this because we pass model_status into the query anyways, which isn't exactly necessary

This comment has been minimized.

Copy link
@alastair

alastair Jun 26, 2019

Author Contributor

I originally had this method for "future use" and we never used it. I'd prefer to implement stuff as we need it, and if we need a variation in the future that we make the change then

This comment has been minimized.

Copy link
@paramsingh

paramsingh Jul 1, 2019

Member

ok, sounds good.


new_all = {}
for cl, val in highlevel["all"].items():
new_all[mapping[cl]] = val

This comment has been minimized.

Copy link
@paramsingh

paramsingh Jun 24, 2019

Member

Sounds ok to me.

@@ -19,4 +19,96 @@ INSERT INTO model (model, model_version, date, status) VALUES ('timbre', 'v2.1_b
INSERT INTO model (model, model_version, date, status) VALUES ('tonal_atonal', 'v2.1_beta1', now(), 'show');
INSERT INTO model (model, model_version, date, status) VALUES ('voice_instrumental', 'v2.1_beta1', now(), 'show');

UPDATE model SET class_mapping = '{

This comment has been minimized.

Copy link
@paramsingh

paramsingh Jun 24, 2019

Member

Is there a reason why we're not inserting mappings for gender, voice_instrumental, timbre and ismir04_rhythm? Would that json be trivial? I think it might be a good idea to make the class_mapping column NOT NULL and insert mappings regardless?

This comment has been minimized.

Copy link
@paramsingh

paramsingh Jun 24, 2019

Member

Then we could also just put this inside the insert statement which might look a bit cleaner?

This comment has been minimized.

Copy link
@alastair

alastair Jun 26, 2019

Author Contributor

The idea is to turn class codes -> real text. In my view, the ones that you said as examples already have names that are nicely readable.
In the future, when we release models built from the dataset editor, I expect that we'll name the classes as nicely readable words (there's no reason that we have to use specific short codes). This means that I expect that we won't need a mapping for any models going forward from here, it's just a quick measure for the existing models that we have.

This comment has been minimized.

Copy link
@paramsingh

paramsingh Jul 1, 2019

Member

Alright, makes sense.

db/data.py Outdated Show resolved Hide resolved
alastair added 3 commits Jun 10, 2019
Most of this work was originally done by @pulkit6559 in
#330
but I made some improvements to minimise the number of sql queries
and simplify some logic.

Add more mappings for existing models
@alastair alastair force-pushed the hl-mapping branch from 58c9d8a to cfd439d Jun 26, 2019
@alastair alastair merged commit cfd439d into master Jul 1, 2019
1 check passed
1 check passed
Jenkins Build finished.
Details
@alastair alastair deleted the hl-mapping branch Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.