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

Render Gene view using JSON #743

Open
4 of 5 tasks
Arnedeklerk opened this issue Mar 31, 2023 · 17 comments
Open
4 of 5 tasks

Render Gene view using JSON #743

Arnedeklerk opened this issue Mar 31, 2023 · 17 comments
Assignees
Labels
code review UX & UI User experience and user interface updates

Comments

@Arnedeklerk
Copy link
Member

Arnedeklerk commented Mar 31, 2023

Part 2 of 3. See 1 here.
Gene view to be updated to parse the returned JSON.

@Arnedeklerk
Copy link
Member Author

@marco-brandizi, @lawal-olaotan has finished the first step of this 3 part process. When you get the time, please take a look at step 1-3 on this ticket.

@marco-brandizi
Copy link
Member

I'm aware of this, but it's not so simple. Changes remained in a branch for long time, meanwhile a lot changed (eg, config system). I need to find time to manually copy-paste the JSON output part.

@marco-brandizi
Copy link
Member

Also, the code about infinite scrolling was way too poor yesterday, will check if it changed much, but I doubt.

@Arnedeklerk
Copy link
Member Author

Cool, I know Lawal reviewed his work, made changes, then worked through it again and sent out more changes... #751

@marco-brandizi
Copy link
Member

Those bugs are a minor aspect, a major concern is the code is still too poor, convoluted, very unreadable, even for something that we plan to replace in not so long time.

@marco-brandizi
Copy link
Member

The API part is ready and merged to master, see #655.

==> Note the trick I've used to be able to push on master (in order to avoid the related API changes to go outdated again):

I've added some Javascript code (see geneTable2OldString() and evidenceTable2OldString() data-utils.js), which takes the new JSON and converts it back to the old TSV-in-a-string. This is a temporary hack, it makes the UI a bit slower and we shouldn't wait for too long before adapting the Javascript code to the new format. See the notes in the mentioned files regarding details about such adaptation.

@Arnedeklerk
Copy link
Member Author

Great that this is working so far!

Part of this ticket is to still return P val and Distance for each evidence. @marco-brandizi when you get time please.

Then we can move to #692, which I believe @lawal-olaotan has started making mock ups for and is still using mock-data.

@marco-brandizi
Copy link
Member

marco-brandizi commented Jul 14, 2023

@lawal-olaotan @Arnedeklerk @KeywanHP

Now the API is returning the topological distance for each concept. As discussed, I had to change the output JSON, now it looks like this and the difference is in conceptEvidences, which reports objects, not just label strings.

As agreed, we can use "score" and "graphDistance" for filtering the gene table, for now, we're going to omit the evidence-related p-value, due to the difficulty of computation.

@Arnedeklerk
Copy link
Member Author

Cool, sounds good to me @marco-brandizi, thanks :).

@lawal-olaotan to you!

@Arnedeklerk
Copy link
Member Author

Working and data as expected. Thanks.

@marco-brandizi
Copy link
Member

@Arnedeklerk Point 5) in the description isn't completed. Keep this open until that.

@Arnedeklerk
Copy link
Member Author

@marco-brandizi I believe this is done, my bad for not ticking it though. Point 5 is about rendering gene view using the Json. Then it refers to other tickets which require that format. Lawal is currently working on: #692

@Arnedeklerk
Copy link
Member Author

Arnedeklerk commented Jul 17, 2023

For future reference: we opted to use the Distance (available for each evidence per gene) and (instead of P val), the KnetScore (@marco-brandizi that's what the Score ended up being, by the way).

An additional API change now also changes the format to return each evidence's distance from the seed gene[s]. I think the commit above broke Ci-test but @lawal-olaotan is working on a fix. Once that's done, we can complete...

@marco-brandizi
Copy link
Member

marco-brandizi commented Jul 17, 2023

I don't see any gene view that uses the new JSON format from the API. Before closing a ticket, the corresponding changes must be at least pushed to github, in most cases should be tested too (on CI).

"Done on my laptop" is not like done.

@Arnedeklerk
Copy link
Member Author

I must be mistaken. Best let's wait for @lawal-olaotan to comment.

@lawal-olaotan
Copy link
Contributor

I should have referenced the ticket when I pushed the changes that adjusted the gene view to the new JSON format because I discussed @Arnedeklerk during our meeting; that was the last step to finish the ticket.

@marco-brandizi
Copy link
Member

I needed to merge my fix for this and other changes around. I discovered another issue, see #777

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code review UX & UI User experience and user interface updates
Projects
None yet
Development

No branches or pull requests

3 participants