Colors in GWSS plots

[jonbrenas]

Adresses #583.

This commit works on the first point, more discussion is needed for the rest.

[leehart]

Thanks @jonbrenas . Just checking whether this PR is ready for review, or if there is more to do on this part (in which case I might convert it to draft) ?

[jonbrenas]

OK, so it looks like I dreamt that I had added the code to this PR. I am going to fish for it.

[jonbrenas]

There are still some issues. Some are pretty minor (the colour map is hard-coded within the function which is dumb, the same needs to be done for the other GWSS functions, ...) but there is something that we need to discuss: the "different colours for different contigs"-thing and the "users can choose how their plots look"-thing don't work particularly well together yet and we need to decide what the parameters should look like.

[leehart]

Thanks @jonbrenas . Yeah, it would be nice to get the default color_dict into a better place. I wonder if there are existing colour palette mechanisms that can be copied, for consistency? I notice plot_ihs_gwss_track supports a palette arg:

palette: ihs_params.palette = ihs_params.palette_default,

palette_default: palette = "Blues"

Regarding supporting both alternating contig colours and user-specified settings, what is the current situation with this PR's code? For instance, if the user doesn't specify any circle_kwargs then I guess plot_g123_gwss_track() should show contigs in alternating colours. Whereas, if the user does want to override the default behaviour, even just to change the colours, then I guess they would specify circle_kwargs accordingly, and they would need to know which settings. Is that right? Are you able to provide a couple of examples here, or maybe we need to chat about that specifically?

[jonbrenas]

Thanks @jonbrenas . Yeah, it would be nice to get the default color_dict into a better place. I wonder if there are existing colour palette mechanisms that can be copied, for consistency? I notice plot_ihs_gwss_track supports a palette arg:

palette: ihs_params.palette = ihs_params.palette_default,

palette_default: palette = "Blues"

The situation is slightly different with iHS as you want a palette with very similar colours to represent the decreasing amount of haplotype homozygosity. Here, we want five clearly distinct colours. So, yes, eventually, the colours will be coming from a palette (the current values are, basically, category10[5] already) but it will be a different one from the one used for iHS.

Regarding supporting both alternating contig colours and user-specified settings, what is the current situation with this PR's code? For instance, if the user doesn't specify any circle_kwargs then I guess plot_g123_gwss_track() should show contigs in alternating colours. Whereas, if the user does want to override the default behaviour, even just to change the colours, then I guess they would specify circle_kwargs accordingly, and they would need to know which settings. Is that right? Are you able to provide a couple of examples here, or maybe we need to chat about that specifically?

I updated the situation a little bit with my last commits but the idea is that there is a default list of circle_kwargs for each contig (only the line_color is actually different though). The user can provide a dictionary with alternative circle_kwargs that would be used instead. Because the contigs are numbers as far as the code is concerned, I will write a short function that converts a contig string into the correct contig int and populates the dict if only one circle_kwargs is given for all contigs.

[alimanfoo]

Hi @jonbrenas, thanks so much, I think this is moving in a good direction :)

We could simplify a little further here I think, because rather than create new functions h12_gwss_contig() and h1x_gwss_contig() I think it would be fine just to change the return value from the existing h12_gwss() and h1x_gwss() functions to add the contig values as a third array.

Most users will rarely if ever call these functions directly, rather they will call plot_h12_gwss() instead, and so changing the return values of h12_gwss() and h1x_gwss() I don't think will impact much.

[jonbrenas]

Thanks @alimanfoo . I indeed created these functions to be backwards-compatible with the idea that we would move to the solution you proposed for the next major release (I thought that it was the idea, that minor releases and patches do not break any previously working code but that major releases can). That said, I am not sure I would have remembered ;) !

[alimanfoo]

Thanks @alimanfoo . I indeed created these functions to be backwards-compatible with the idea that we would move to the solution you proposed for the next major release (I thought that it was the idea, that minor releases and patches do not break any previously working code but that major releases can). That said, I am not sure I would have remembered ;) !

Thanks @jonbrenas. I'm happy to change the API and make a major release release. In general I think making a major release is fine, the main thing is to be aware that we have made an API change and so a major version bump is needed. Also it is good to think about whether the API change will impact lots of user code, and so only do it if there is clear value. But in this case I think the impact to user code will be minimal.

[jonbrenas]

Thanks @alimanfoo. This should be better now.

[leehart]

@jonbrenas to resolve conflicts and provide examples here and in notebooks.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[jonbrenas]

Here are some pictures from the new cells in the notebook:

H12 (default colours):

H1X (manual choice of colours):

I didn't modify the part for Af1 as there is no option for multiple contigs (we might want to colour 2L and 2R differently for example but that would be more complex because they are 1 contig for funestus).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.19%. Comparing base (23c0908) to head (af6bac1).
Report is 30 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #591      +/-   ##
==========================================
- Coverage   95.66%   95.19%   -0.47%     
==========================================
  Files          39       40       +1     
  Lines        3921     4144     +223     
==========================================
+ Hits         3751     3945     +194     
- Misses        170      199      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jonbrenas]

@alimanfoo, @leehart, I am having trouble with codecov. I don't think the part that is not covered is an issue as it is exclusively code that was already in master.

[leehart]

@jonbrenas I think we have ignored and merged regardless before, e.g. #647, but I reckon it's a persistent issue, i.e. #554

[alimanfoo]

Here are some pictures from the new cells in the notebook

Awesome, looking good!

Could you post an example of what it looks like with default colours if you do region="23X" just for interest, i.e., whole genome.

[alimanfoo]

Looks great, just a couple of really tiny suggestions.

[alimanfoo]

Suggested change

	name = "h12_gwss_contig_v1"
	name = "h12_gwss_v2"

Nit, convention is to use the function name then add a version number.

[alimanfoo]

Suggested change

	name = "h1x_gwss_contig_v1"
	name = "h1x_gwss_v2"

[alimanfoo]

Do you notice any performance difference at all from this change? Just curious, if you aren't noticing any obvious slowdown then I'm not worried.

[alimanfoo]

@jonbrenas I think we have ignored and merged regardless before, e.g. #647, but I reckon it's a persistent issue, i.e. #554

Yeah I think we can ignore the codecov/project check for now. The main thing is that the codecov/patch check is passing, which shows you have good coverage of the code that is being changed in this PR.

A previous PR reduced overall coverage for the project slightly, and the codecov/project check keeps complaining about this in every subsequent PR. Feels like there should be some way to reset the base for the codecov/project check, but not sure how.