Verify that RHS snapshots correctly reproduce the captured state

[mwinokan]

@boriskovar-m2ms to verify after the RHS changes #1404 and #1348

[mwinokan]

@mwinokan to spend some time generating example snapshots and see how they perform

[boriskovar-m2ms]

Here is my list of found issues after analyzing 200+ snapshots:

• (1) Selected compounds - checked/unchecked state of the color filter is not preserved
• (2) LHS doesn't scroll to the first seleced pose
• (3) LHS - select all hits is not preserved
• (4) Select displayed hits doesn't work and doesn't restore
• (5) Select all tags - the function of the button (selecting all tags) is restored but the state of the button is not
• (6) 'observations of a pose' window is not opened
• (7) RHS - if you have for example 3 ligands on and start iterating, it will asks if you want to lock rest of the ligands. Click yes and start iterating. Save snapshot and after restoring the selected ligand which is not locked (basically iterator) is not restored. Only locked compounds are restored.
• (8) incorrect rendering of snapshot (orientation, contours wrong) #1303 (comment)

Identified but not relevant (says Frank)

• (was 7.) Vector selector doesn't scroll to first selected compound
• (was 11.) When you add representation (so single object e.g. ligand has two representations like ball+stick and axes) and remove it after then in the snapshot there are still both representations
• (was 12.) When you edit representation the representation is restored in the NGL view but the edited values of the representation are not preserved in the representation edit dialog

Other findings related to representations and vector selector:

• Vector selector is not opened - sometimes - like this snapshot https://fragalysis.xchem.diamond.ac.uk/viewer/react/projects/50/52 but in this snapshot it works https://fragalysis.xchem.diamond.ac.uk/viewer/react/projects/51/53
• Visibility of a representation is not preserved

[mwinokan]

@boriskovar-m2ms says the issues all arise because the action doesn't exist, action data is out of date, or the restoration process is broken.

12 might be more time consuming though, because the representation is being stored rather than the values. The custom representation dialog has been made by M2M and @phraenquex says they are non-intuitive and 12 should be a new ticket.

10-12 have been covered by #1462

7. vectors can also be left out for now as we need to rethink the implementation/spec

[boriskovar-m2ms]

This is not snapshot related but Select All Tags is not recorded as a mass action but as 20 Select tag actions (or whatever number of tags we have in a target) so undo redo unselects and selects only individual tags. Skipping for now.

[boriskovar-m2ms]

Point 6 is broken also for RHS for inspirations. Thought I was going to reuse it for LHS but it seems that it was actually never implemented for RHS and preliminary analysis shows that implementation is not easy because there is a lot of moving parts and also lot of coroner cases like for example but not only we are scrolling to first visible item but what if we have a this dialog open but it would be out of view and so on and so on.

[boriskovar-m2ms]

Point 7 is very very deep rabbit hole. For this one we need also point 6 implemented. We already have a mass actions when same action is performed on many objects of the same type and undo and redo should undo and redo all of these actions in one step. But this is a new kind of actions which I would call a composite action which means that there are performed many different actions on many different objects and they should be look like single action from the user point of view (just like mass action).

[mwinokan]

@phraenquex says that @boriskovar-m2ms can skip the undo/redo functionality for the deep rabbit hole for point 7.

@boriskovar-m2ms says we still need point 6 to restore the RHS state, but please check if there is a workaround.

@boriskovar-m2ms has also thought about storing and restoring the state instead of storing the list of actions, off the shelf libraries for getting differences between states (for undo/redo) won't work because for us.

@boriskovar-m2ms to fix the user-facing issues but also document the recommended refactoring that would improve performance in the long run.

[boriskovar-m2ms]

@mwinokan I fixed directly in your comment that we need point 6 and not 7 to restore RHS. And also unfortunately there is no workaround.

[mwinokan]

@boriskovar-m2ms now has new test data, and it has revealed the need for some more work. Especially differentiating between the selected compounds and compound sets tabs.

@boriskovar-m2ms will also have to reconcile with @matej-vavrek's various changes.

@boriskovar-m2ms please keep track of a list of things you are regularly testing as part of this, so that we can compile a list of standard tests to be carried out after large changes (and potentially as part of an automated test suite).

@boriskovar-m2ms also raises that extremely long source files cause issues in visual studio code, which significantly slow down development work. E.g. saving files with 1000's of lines can take several minutes or even time out, debugging is also broken. @phraenquex stresses that the mint release is less time-sensitive than previous releases and a good opportunity to do house keeping.

[boriskovar-m2ms]

@mwinokan I fixed the problem when you had dialog with inspirations opened in selected compounds tab and started iterating and after that you would save a snapshot and when you restore snapshot it (dialog) would be attached to different compound than it should be.

BUT another thing that I noticed is that the functionality to scroll to the first visible compound in selected compounds tab when restoring snapshot was actually never implemented (it works now for LHS and RHS computed sets). I guess this should fixed too.

Here it's not going to be a simple port (but also not something super hard) of the functionality because the list of compounds in here has quite different structure because for the fact that compounds might come from different computed sets.

[boriskovar-m2ms]

While implementing auto scrolling to first visible compound (problem described in previous comment) I found yet another bug. This time it's 4 years old bug when you have custom pdb and have compounds from two different datasets but they have same ID then they're treated as a same compound.

[boriskovar-m2ms]

Also there is a more recent bug that auto scrolling to first visible compound in custom dataset when using reference to LHS pdb is not working (it's hard to test because pressing P button on any of the compounds lights them up all).

[boriskovar-m2ms]

While implementing auto scrolling to first visible compound (problem described in previous comment) I found yet another bug. This time it's 4 years old bug when you have custom pdb and have compounds from two different datasets but they have same ID then they're treated as a same compound.

Just found out that this is not a bug because according to DB table the computed compound id is a primary key so it will never by same. Got confused by this 4 years old code which instigated this research because it was handling the case that two computed compounds from two sets can indeed have same id. In this case we would need to rework nice chunk of RHS to handle this.

[mwinokan]

@boriskovar-m2ms has made good progress fighting snapshot gremlins and is now testing his changes.

Boris says there are still issues with restoring the scroll state for the selected compounds tab (currently the wrong tab is opened).

Additionally, VS code's automated refactoring tools were not able to split the largest source code files so it will need to be done manually. @boriskovar-m2ms says that the refactoring would be nice to have but not necessary. @phraenquex recommends we do our due diligence and include the source code refactor and unit tests within this ticket.

[boriskovar-m2ms]

@Waztom @phraenquex @mwinokan
If we want to switch current action based snapshots for state based snapshots, this is how it should work.

Saving snapshot:

1. We create copy of the state and from the copy we remove everything that is "downloadable" i.e. poses, observations, tags and all computed datasets, job definitions
2. We send the state json to B/E (in current implementation snapshot entity also has free form json field additional_info so theoretically we can use this one so we do not need to change B/E)

Snapshot restoration:

1. We download the state (or extract it from additional_info from current snapshot object)
2. Download all the stuff we stripped away from state during saving
3. All NGL viewer stuff will be restored by multiple useEffect
4. All more esoteric stuff like scroll to first visible compound will be also implement by useEffect idiom
5. Incompatibilities between version will be also handled by useEffect or preprocess pipeline if necessary
6. In project view where you can switch between snapshots it will be handled in a way that we first apply new NGL view orientation matrix and then apply new state so it will first continuously rotate and after that the changes will be applied (or other way around - don't remember what is done first. If rotation or applying the changes)

PROS:

1. The biggest pro is that after initial implementation the snapshot saving and restoration will require no additional work (currently you need to specify for every small action: the action object, save behavior, restore behavior, redo and undo behavior). This requires that the NGL viewer will be controlled only using useEffects which will monitor changes in the state related to NGL viewer. Currently it's done imperatively after you lets say press L button we change the state and we tell NGL viewer in the same function what to render. The information about ligand is used only by the rest of the frontend. Now we will implement useEffects which will fully control NGL viewer so NGL viewer will only react to changes in global state and we will never directly control it. The only additional work will be with actions like scroll to first visible compound but historically we do not do this very often.
2. Waaaaay less code needed therefore less code to maintain and need to understand
3. Easier to understand code (some parts will definitely not be trivial but on aggregate it will be easier to understand)

CONS:

1. We will most likely lose Undo/Redo functionality. There is a kind of a hope that we will be able to regain some of the ability to undo redo but it will be undo redo on very atomic/low level so no mass actions nor aggregate actions and definitely no NGL viewer only undo/redo (I don't think they fully work even now). So basically if you press button that selects 20 hits then you will need to press undo 20 times to really undo that one action (yes in this specific case it can be done by updating state in one go and not 20 times when pressing the button but in general it not might be possible - specifically when one action is updating multiple different slices of redux store).
2. No actions timeline

Not quite sure how long it would take me to implement this but we are definitely talking about several weeks but most likely we are not talking about multiple months.

[phraenquex]

Thanks @boriskovar-m2ms veryhelpful.

Some questions, and maybe you've implicitly covered them already.

• can you confirm you're sure this will allow you to restore everything? Or are there open questions?

• the work from the last many weeks - is that worth rolling out anyway? Because formally, you've already fixed most of the bugs, and all this new work would "only" address the one remaining one, to restore the scrolling.

• if we postpone this refactoring, can you think of a quick-n-nasty work-around for the scroll bug?

• by when will you have an estimate of the time to delivery? (Wall time, not hours billed.)

• Undo-redo: can you retain this in the session - so no need to restore into the snapshot, but it would be a (big!!!) shame to lose the functionality, given that it already exists. Will that create future technical debt?

My thinking is: we should aim to get a lot of mint out the door ASAP, because we need it for science.

[boriskovar-m2ms]

1. We will be able to restore everything that is stored in state effortlessly. The only stuff that is currently not controlled by state is stuff that cannot be controlled by state explicitly and that being NGL viewer related stuff and browser specific stuff (only thing I can think of is actually the position of scrollbars). Both things will be addressed by using useEffect which will synchronize our state with NGL viewer internal state. Position of the scrollbars will be addressed (actually already is addressed) also by uisng useEffect which will scroll first visible/or with open dialog compound/pose into the view. There is another class of internal state and that is that some stuff (just few fields) we are holding for some reason in the react context and if needed we can move these fields to our redux store/state if needed.
2. The work from last many weeks is done and yesterday I rebased my branch with stuff from staging (some nasty merging so hopefully it did not break stuff but first impression is quite good).
3. The scroll bug is solved or rather lets say "solved". The solution was to disable the functionality that currently when you have dialog with inspirations open and you scroll given compound all the way up/down (i.e. the compound is no longer visible in the list) it closes this dialog. This functionality was that what was fighting against the scroll functionality so I disabled it. The new approach will not solve this issue and special code will be needed to solve this (by using useEffect our trusted friend) BUT the advantage of this approach is that these very special cases are the only cases that you will need to address manually and 90% percent of the stuff will just work out of the box. Currently nothing works out of the box and typical breakdown of work on typical feature is that you spent 2/3 of time on functionality and 1/3 of time on snapshot functionality (sometimes - like iterating using arrows on RHS it's 50-50).
4. Need to talk with Martin and Aneta about my planned utilization and they will be back from holiday after next week. So my guess is within two weeks. In the mean time I can prepare estimation for hours billed and then I will just spread them in time based on intended utilization
5. Undo-redo is solely based on the current action based (and manual and very laborious ) approach so we will lose this functionality if we move to "preserve whole state" approach. I THINK we can use library that is implementing undo-redo by reversing and reapplying state mutations but this can be quite limited like I described in point 1 of CONS of my previous comment i.e. the action that is undone or redone won't be all the mutations that pressing given buttons causes but all these mutations will be undone or redone one by one and not like one atomic unit. I also have some ideas in this area but those are very experimental like for example we can modify such a library in a way that every value in our state might have also something like action identifier (generated guid) and when you're undoing and redoing a diff where action identifier is present then it will apply all the state diffs which are labeled by same action id so this way we can group state mutations which are caused by single user action. Ideas like this but they would definitely need some prototyping first.

[phraenquex]

Thanks @boriskovar-m2ms - so headline is this?

(a) you've fixed an awful lot, and that's ready for testing. Please change swimlane as appropriate, and @mwinokan will test when back.

(b) you have a solid plan for refactoring - please copy-paste to a new ticket, and assign to orange. We'll schedule it as soon as you know more.

[boriskovar-m2ms]

@phraenquex

1. @mwinokan Merged version is deployed here: https://fragalysis-tibor-default.xchem-dev.diamond.ac.uk/viewer/react/landing/
2. new ticket is here Refactoring snapshots #1483

[Waztom]

Merge and test once #1322 in staging.