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

feature/improve-navigator-ordering #244

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

EliotRagueneau
Copy link
Contributor

Fix 2 problems:

  • Avoid floating predicted complexes components because they are "not found" in any complex (Ndc80 search)
  • Avoid instability in results arising from race conditions between timesAppearing definition and timeAppearing usage

Also improve sorting of components when similar timesAppearing and similar first index appearing.

This improvement needs to be discussed and reviewed because it is significantly longer (+17,52% compute time for a relatively big example (20 complexes x 272 components, still marginal time though from 7ms to 8,5ms)

See comparison at https://docs.google.com/spreadsheets/d/1oG9kVww2F1s5U2EjoataKmOiVaY4yx4dXloqJJGqBAc/edit?usp=sharing

@EliotRagueneau
Copy link
Contributor Author

Previous sorting: Sort by firstIndex, then if equal, by amount appearing
Screenshot 2024-12-13 at 17 19 37

New sorting: Sort by all indexes of apparition

  • if component A appears more time than B, then component A is placed first (keeping the behaviour of gathering shared component at the end for the stairs effect)
  • If they have the same first index of apparition, we compare the second, and then the next ones until there is a difference or we ran over all apparitions. This makes basically sorted first by the first complex they appear in, then by the sceond complex they appear in, etc, creating again a kind of staircase effect
Screenshot 2024-12-13 at 17 21 27

@EliotRagueneau
Copy link
Contributor Author

These are extremely rare conditions though, as you need to have different complexes having the same amount of component shared with another one, but not the same components. Here for instance all components have a first index of 0, so then they are all compared based on the timeAppearing, but they are all appearing only 2 times, so they are considered equal by our sorting algorithm, and therefore don't get sorted between one another.

Therefore, I think the proposed solution is better, but I don't know if it's worth a 20% increase of compute time, what do you think @jmedinaebi ?

Copy link
Contributor

@jmedinaebi jmedinaebi left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of minor comments.

@@ -292,6 +289,7 @@ export class TableInteractorColumnComponent {
}
}
}
return navigatorComponents;
Copy link
Contributor

Choose a reason for hiding this comment

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

In this method, we are updating this.navigatorComponents() and also returning and setting the returned value to navigatorComponentsTA(), so this.navigatorComponents() and navigatorComponentsTA() are equals. We should treat this.navigatorComponents() as immutable, creating a new array and returning the new array.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, are we not using navigatorComponentsTA in the HTML at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just pushed a change for renaming navigatorComponentsTA and also to use the new variable instead of navigatorComponents.

@jmedinaebi
Copy link
Contributor

I have pushed a new commit to show the loading spinner when the user makes a new query, instead of just showing the old results until the new ones come in. I have deployed my changes (this and the previous commit) to the github pages.

I have also restarted Complex-WS and SOLR in the public instances to see if that improves things, as queries were being slow this morning.

… timesAppearing and indexAppearing when they are available, which is after some compute
…ents when really needed, + call search API when really needed + trigger loader whenever we actually request things, instead of relying on data
@EliotRagueneau
Copy link
Contributor Author

#d388b28 Is really important because I realised every time we update the display options or the sorting or grouping, we were triggering a new search API call, which was making everything slow. Thanks to this one, we now send requests only when an actual parameter of the search has changed. Also make sures that we only triggers the preparation of components and the sorting when actually needed, not whenever you change something.

Copy link
Contributor

@jmedinaebi jmedinaebi 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 few comments on the latest changes.

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.

2 participants