PSI-BLAST/HMMER/HHpred/HHomp bugs

[zy4]

• [x] For PSI-BLAST: checkboxes should be checked initially, but only ones with an E-value <= 'E-value inclusion threshold'.
• [x] Select Hit no. 250 in the Aligments section of PSI-BLAST and then go to Hit no. 250 in the table; it should be selected as it is on production presently.
• [x] Select some hits on page 1 of the table and go to some other page. Return to page 1 and the hits are not selected anymore.
• [x] Select a hit on page 5 of the table; it should also be selected in the Alignments section.
• [x] Select a hit in the table and change Show entries; the selection is lost.
• [x] PSI-BLAST: forwarding full-length sequence is broken.
• [x] Clicking on the hitmap does not take one to the unloaded alignments (works on production)
• [ ] Reoccurring off-by-one error in full-length-forwarding without evalue

[zy4]

Also the full-length forwarding for Hmmer seems to be broken

[vikramalva]

Yes. There some other issues as well. I will try to post them in the coming days.

[vikramalva]

• [x] Clicking on the hitmap does not take one to the unloaded alignments (works on production)

If we use datatables for the Alignments section (#112), this would have to be reworked in any case.

[zy4]

/cc @vikramalva ,

I figured something strange: The forwarding checkox "aligned region" does not work on production either. And from the implementation logic it does not work because it has never been implemented. Did this ever work and how?

EDIT: can we remove it from PSIBLAST and HMMER forwarding? I saw that it is not there for HHpred

[zy4]

Now I get it. The aligned region does not allow for selecting via slider, it is simply the opposite for the full-length option. Therefore the variable sliderRegion in normal.scala.html is redundant.

Question: should we enable a feature where the forwardable sequences should be selectable via the slider?

[vikramalva]

It has always worked and still works normally for me. We can't remove it as PSI-BLAST and HMMER forward both aligned and full-length sequences.

[zy4]

I simply misunderstood the functionality of this checkbox. I thought it would grab the current slider situation. But it doesn't. And there is a redundant variable in the code, but simply don't care about this, I will point this out in the next commit.

[vikramalva]

The only job of the slider is to allow forwarding of a part of the query sequence through the 'Resubmit section' button. The 'aligned region' checkbox extracts selected sequences from the alignment, as opposed to extracting the full-length sequences.

[zy4]

I got that. It was not obvious from the code, though.

[zy4]

Clicking on the hitmap does not take one to the unloaded alignments (works on production)

works not 100%: after clicking, the reload happens but no scrolling to the hit (using chrome)

[vikramalva]

It does not work perfectly; click a second time.

[zy4]

It took me a while to make up my mind about the remaining issues which are all connected as being checkbox persistence issues.

You can see my hesitance also in the fact that I didn't knew how to rewrite the linkCheckbox function which is the last remnant of blast-visualization.js

Initially I wanted to avoid mutable states in the views, especially global variables but it is not easy to go without mutability in this case.

I am about to finish this but before I can do, I need an answer considering the kind of persistence we want. Imagine you visited a result page once. Leaving the page deletes all check states from the boxes. Should we keep the state up to the point that reloading the page would retain this information as well?

[felixgabler]

I don't think it should persist over reloading.

[zy4]

I am not sure about this because reloading and switching between jobs are not the same in a SPA (so I am talking about redraws rather than reloads). It might come in handy to have some persistence for comparative reasons when working with multiple jobs on one task.

[vikramalva]

I would keep it simple and let the selections persist only on the active page. They should be reset when one reloads or leaves the page. Not resetting might lead to situations where one makes new selections and also has previous ones selected unknowingly.

[anjestephens]

Is there any progress on this? Can Marko or I help somehow?

[zy4]

It is only the checkbox handling which is difficult. I would appreciate to hear some architectural proposals from you. I really thought a lot about this and couldn't find a solution which isn't ugly and which does not add up to our pile of bad code.

The problem is that we don't have a proper state handling which would employ some rx pattern like redux. But I guess there must be a custom fix for our case where we would not migrate our complete frontend code to a framework like angular, which we shouldn't do anyway because of performance.

So the questions that arose for me are:

1. should we get rid of twirl
2. how to manage state
3. do we need a framework for this?

the old solution simply stored global variables in the template as a state storage. This led to strange code like a linkcheckbox function and much unmaintainable code which had many bugs and code duplication all over the place. I totally want to do without such hacks. For this we should use either the observer pattern or a state-graph approach like ScalaRX.

[zy4]

I worked on this again over the course of the weekend and I think I can get it done this week with rewriting the frontend (only the twirl templates, not the mithril typescript codebase). This is a bigger task, but it is better to do it that way than piling up on bad code.

[felixgabler]

I fixed the checkboxes stuff and improved the code quality. I still want to work on it however. Can be implemented in a more stable way. But for now we can at least merge with master after testing

[zy4]

I merged your branch and I also merged master into develop so that develop could directly be merged into master. Can you test the devlop branch, @vikramalva ?

[vikramalva]

Closing this in favor of #346