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

ChIA-PET loops display and exploration contribution #1445

Merged
merged 4 commits into from
Dec 29, 2021

Conversation

cheehongsg
Copy link
Contributor

Dear IGV.js team:

Thanks for your continued effort in enhancing the IGV, igv.js and IGV-webapp.

Dr Wei's team (https://www.jax.org/research-and-faculty/faculty/chia-lin-wei) will like to share the following enhancement to igv.js interaction view which can be helpful to visualise ChIA-PET loops.

chiapet-loops

It is also common for us to flip the y-axis for the ChIP-Seq wig signal to show support for the loop anchors. (This is similar to the "toggle arc direction" option in interaction track.)

The track label popup will show additional track metainfo.

The semi-colon removal is enforced by eslint.

My apology for not having split these into individual standalone pull request.

Cheers!

@jrobinso
Copy link
Contributor

jrobinso commented Dec 23, 2021

@cheehongsg thanks for the PR. It looks good overall, I have just a few comments/requests

(1) removing the semicolons is a good thing, but results in a number of files being changed which are unrelated to the substance of this PR. I prefer to leave those changes out, which means rolling back the first commit.

(2) We will need some description of "ChIA-Pet 2A" and "ChIA-PET 1", etc, options as this isn't going to be obvious to everyone. I see the description on the screenshot, but its not real clear what that means. Also, should these options be shown all the time, or just when viewing ChIA-PET data? If these options apply generally to all types of arcs perhaps we could use some more generic names.

(3) The change to the track description popup is too disruptive to apply generally, some tracks have a lot of config parameters many of which make no sense to an end user. Also, the track "config" is not treated consistently thoughout the code, it can get modified for some track types. This is a general problem I am aware of, not related to your PR. If the display of this meta information is important for your tracks we could enable it with a configuration option, perhaps a magic string for the description field such as description: $FULL$. I also note that you can accomplish this with your own tracks by supplying a function argument for the "description" field, although this approach will not work with sessions. By that I mean the function will not be stored with a session, which includes bookmarks and "shared" links.

@cheehongsg
Copy link
Contributor Author

(1) I will this roll back.

(2) You are right, the options should be more generic and relatable. They are applicable to all (cis-)interaction loops and will allow users to focus on the different classes of interactions. This chart will be more informative. What are good generic names that are relatable to the 3D chromatin community?

arc-types

(3) I agree that the popup can be disruptive for general application.

We implemented this based on issues that users faced when working with large number of tracks. Take the screenshot below, it was impossible to tell the differences between the two tracks loaded from "ENCODE Other ..." (See region A.) This popup will display the metainfo available from the columns of the "custom-data-modal" table configure for consumption of igv-webapp. (Region B.) Embedding the "critical" meta-info in the track label does waste estate space.

track-meta-info

Do you have a recommendation for such scenario?

I can roll (3) back and open a separate PR for it.
In this way, we can move forward with (2) after finalising the labels and its general applicability.

@jrobinso
Copy link
Contributor

@cheehongsg All good points, the new figures are informative. Let me give some thought to issues 2 and 3, i.e. names for the menu items and also better descriptive popup text for the track. I do accept there is a need for that, I would like to consider ways to do it generally for all tracks. This will require a little thought.

Officially we are on holiday starting tomorrow until Jan 2, but I expect to work some in that time. However, on the other hand I might not :). If we don't get to this will do it first week in January, have a nice holiday.

@cheehongsg
Copy link
Contributor Author

@jrobinso Thanks for thinking them through.

It is very beneficial to have breaks. Cherish them. Happy holidays to the IGV team!

@jrobinso jrobinso merged commit 7dc4a80 into igvteam:master Dec 29, 2021
@jrobinso
Copy link
Contributor

@cheehongsg In case you're working this week... I'm looking at generalizing the "ChIA-Pet" options. Here's an idea, rather than lump the filtering of arcs (all, one end in view, both ends in view) with the arc type (nested or proportional) perhaps these should be independent choices. That is you choose an arc type, then select the arcs you want drawn in terms of what you want to see (i.e. all, within, or partial). I found it disconcerting when selecting the ChIA-Pet options that the arcs switched from nested to proportional. Is there a use case for drawing "within" as nested arcs?

I find this easier to conceptualize but maybe there is no use case. The nested option is of most interest for RNA structure drawings, I'm not sure its all that interesting for chromatin interactions.

jrobinso added a commit that referenced this pull request Dec 29, 2021
@jrobinso
Copy link
Contributor

For now I decided to just re-label the track items as shown below. We can revisit the options later. These labels are not of course final, just a first pass, I am trying to be explicit on what the option actually does.

Screen Shot 2021-12-28 at 9 58 54 PM

@jrobinso
Copy link
Contributor

@cheehongsg Is there a reason we need both drawProportional and drawProportionalChIAPET? These seem mostly the same, but the ChIAPET options filter some of the arcs out dependending on their endpoints. I can combine them, but am asking if there are other subtle differences I am missing.

@cheehongsg
Copy link
Contributor Author

@cheehongsg In case you're working this week... I'm looking at generalizing the "ChIA-Pet" options. Here's an idea, rather than lump the filtering of arcs (all, one end in view, both ends in view) with the arc type (nested or proportional) perhaps these should be independent choices. That is you choose an arc type, then select the arcs you want drawn in terms of what you want to see (i.e. all, within, or partial). I found it disconcerting when selecting the ChIA-Pet options that the arcs switched from nested to proportional. Is there a use case for drawing "within" as nested arcs?

We have not been using "nested arcs" in ChIA-PET data visualization. Nested arcs emphasise which two "blocks" of chromatin are in proximity but lack the frequency information. "Within" is to focus user on chromatin proximity in the current genome span.

I find this easier to conceptualize but maybe there is no use case. The nested option is of most interest for RNA structure drawings, I'm not sure its all that interesting for chromatin interactions.

I concur.

@cheehongsg
Copy link
Contributor Author

For now I decided to just re-label the track items as shown below. We can revisit the options later. These labels are not of course final, just a first pass, I am trying to be explicit on what the option actually does.

Screen Shot 2021-12-28 at 9 58 54 PM

They are apt labels. Thanks.
Let's go with these and adjust if the community has a better label suggestion later.

@cheehongsg
Copy link
Contributor Author

@cheehongsg Is there a reason we need both drawProportional and drawProportionalChIAPET? These seem mostly the same, but the ChIAPET options filter some of the arcs out dependending on their endpoints. I can combine them, but am asking if there are other subtle differences I am missing.

@jrobinso : Thanks for the careful review.

The only reason is to facilitate updating our in-house repository with igv.js official commits while developing in parallel. (E.g. feature might not be accepted officially, but we wish to maintain its availability in our installation.)

The only subtlety is drawProportionalChIAPET did not implement any trans-interaction rendering. Most researchers do not make use of ChIA-PET trans-interactions, thus its exclusion in the code.

We have been extracting useful information from ChIA-PET trans-interactions and will share this customisation shortly with another pull request. For the time being, you may use the current trans-interaction handling in drawProportional when merging the two functions. (See page 6 for sample output and page 7 for the customised output in "Exploring ChIA-PET cis-and-trans interactions in IGV WebApp.pdf".)

@jrobinso
Copy link
Contributor

@cheehongsg OK. I can wait until your next PR before doing further work (combining the draw functions), to avoid merging problems, it is functional as is.

jrobinso added a commit that referenced this pull request Dec 29, 2021
@jrobinso
Copy link
Contributor

@cheehongsg I restored your track description popup, actually reimplemented and cleaned up track description code a bit. I had not noticed on the first review that the added popup had an "is capitalized" test, this alleviates the concern of overloading the user with irrelevant properties. Apologies for missing that, I find reviewing code in my head difficult sometimes. I also restored the wig track flip-axis addition, which had somehow gotten dropped. So I think everything from this original PR is implemented, if not we will handle with new bug reports or PRs.

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.

None yet

2 participants