Skip to content

Redbiom improving listing #2132

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

Merged
merged 8 commits into from
Jun 7, 2017

Conversation

antgonza
Copy link
Member

Changes based on discussion during our 05/10/17 meeting. Basically, we are grouping by parent (trim, split, etc).

Note that there are different ways to do this: (A) changes in the db so it's "easy" to retrieve the default setting name from parents or any artifact, (B) do the matching in SQL or (C) python. (A) will need a lot of changes to the code and the db, and we will need B or C to make it happen as we will need to populate all current artifacts. Thus, decided that (B) or (C) were the option. Started with (B) and realized that all json operation in postgres (the processing parameters are stored as json) are not easy (or even possible) in any version below 9.4; in fact, greater of 9.5 will make life easier; our servers have 9.3. Even so, tried this way and the query was taking around 12 seconds from the 'infant' test (it originally took ~500ms) so discarded that version. Thus, implemented (C) and it's the one in this PR ('infant' test takes ~1s, not terrible).

Will deploy in the test environment in a few minutes.

@antgonza antgonza mentioned this pull request May 12, 2017
9 tasks
@antgonza antgonza requested review from wasade and josenavas May 12, 2017 18:03
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.009%) to 91.967% when pulling 58edff2 on antgonza:redbiom-improving-listing into 5dc2b40 on biocore:redbiom.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 91.958% when pulling 58edff2 on antgonza:redbiom-improving-listing into 5dc2b40 on biocore:redbiom.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 91.958% when pulling 8a2a167 on antgonza:redbiom-improving-listing into 5dc2b40 on biocore:redbiom.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 91.958% when pulling 9fa79e2 on antgonza:redbiom-improving-listing into 5dc2b40 on biocore:redbiom.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 91.958% when pulling bfb7efc on antgonza:redbiom-improving-listing into 5dc2b40 on biocore:redbiom.

Copy link
Contributor

@wasade wasade left a comment

Choose a reason for hiding this comment

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

Mostly just clarifications

at.artifact_type_id = a.artifact_type_id
AND artifact_type = 'BIOM')
ORDER BY artifact_id
GROUP BY study_title, study_id, artifact_id),
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to get an english description of what this query does? inline comments within the query to describe the subqueries would also be helpful

Copy link
Member Author

Choose a reason for hiding this comment

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

added some information.

for row in qdbsc.TRN.execute_fetchindex():
title, sid, samples, name, cid, aid = row
title, sid, samples, name, cid, aid, pp = row
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont understand how the select on main_query did not change but somehow row has an additional element now?

Copy link
Member Author

Choose a reason for hiding this comment

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

the with is basically a select after another select, this allows us to make faster queries and "work" with json ... hopefully the explanation added helps.

# [0] then taking the first element that is
# the name of the parameter set
ppc = sorted(
[[k, len(eval(pp) & v)]
Copy link
Contributor

Choose a reason for hiding this comment

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

what is being evald?

Copy link
Member Author

Choose a reason for hiding this comment

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

psql return a set as a string so we are eval that ..

nr['command'] = commands[cid]['cmdn']
nr['software'] = commands[cid]['sfwn']
nr['version'] = commands[cid]['sfv']
commands[cid] = '%s - %s v%s' % (
Copy link
Contributor

Choose a reason for hiding this comment

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

should trim length be represented?

Copy link
Member Author

Choose a reason for hiding this comment

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

it has it, in the next block the processing parameters of the parent are added.

Copy link
Member Author

@antgonza antgonza left a comment

Choose a reason for hiding this comment

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

thanks, @wasade, I think my comemnts should help, please let me know otherwise. @ElDeveloper or @josenavas could you take a look?

at.artifact_type_id = a.artifact_type_id
AND artifact_type = 'BIOM')
ORDER BY artifact_id
GROUP BY study_title, study_id, artifact_id),
Copy link
Member Author

Choose a reason for hiding this comment

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

added some information.

for row in qdbsc.TRN.execute_fetchindex():
title, sid, samples, name, cid, aid = row
title, sid, samples, name, cid, aid, pp = row
Copy link
Member Author

Choose a reason for hiding this comment

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

the with is basically a select after another select, this allows us to make faster queries and "work" with json ... hopefully the explanation added helps.

nr['command'] = commands[cid]['cmdn']
nr['software'] = commands[cid]['sfwn']
nr['version'] = commands[cid]['sfv']
commands[cid] = '%s - %s v%s' % (
Copy link
Member Author

Choose a reason for hiding this comment

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

it has it, in the next block the processing parameters of the parent are added.

# [0] then taking the first element that is
# the name of the parameter set
ppc = sorted(
[[k, len(eval(pp) & v)]
Copy link
Member Author

Choose a reason for hiding this comment

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

psql return a set as a string so we are eval that ..

Copy link
Contributor

@josenavas josenavas left a comment

Choose a reason for hiding this comment

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

Just a question. Also, is it possible to put the dropdown choosing the processing parameters on top rather than on the bottom? It's location inside the table is not ideal, but I guess it will take too long to pull it out. Also just noticed that the label on the table filter "Filter results by column data (Study Name, Artifact Name, Software, etc):" is not synced with the actual column names.

* gif to show that the section of page is loading
*
*/
function show_loading(div_name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this function removed from here?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the previous PR, you requested to be moved here and I did. However, while working on this version I realized that it was a bad decision because it uses {% raw qiita_config.portal_dir %}, which it's not executed correctly as it's in the qiita.js file. Moved it sitebase.html so it works correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.009%) to 91.967% when pulling 95c80c2 on antgonza:redbiom-improving-listing into 5dc2b40 on biocore:redbiom.

Copy link
Member Author

@antgonza antgonza left a comment

Choose a reason for hiding this comment

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

@josenavas, fixed your comments.

* gif to show that the section of page is loading
*
*/
function show_loading(div_name) {
Copy link
Member Author

Choose a reason for hiding this comment

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

In the previous PR, you requested to be moved here and I did. However, while working on this version I realized that it was a bad decision because it uses {% raw qiita_config.portal_dir %}, which it's not executed correctly as it's in the qiita.js file. Moved it sitebase.html so it works correctly.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 91.958% when pulling 8abab71 on antgonza:redbiom-improving-listing into 5dc2b40 on biocore:redbiom.

* gif to show that the section of page is loading
*
*/
function show_loading(div_name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@wasade wasade merged commit b37564c into qiita-spots:redbiom Jun 7, 2017
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 this pull request may close these issues.

4 participants