Unify colour & display name setting for all models #67
Conversation
|
|
||
| const variantColors = new Map([ | ||
| ["other", "#777777"], | ||
| ["22B", "#999999"], |
There was a problem hiding this comment.
22B, 22D and 22E were not copied over to the new configuration as they were not present in the latest model runs.
There was a problem hiding this comment.
I very much appreciate the centralization and moving colors into the JSON. Can I request one change however... the rest of workflow is quite good at putting everything you need to modify into config/config.yaml. The CLADES entry in modify-lineage-colours-and-order.py is now the exception and requires modifying the script itself. Can you move the CLADES entry into config/config.yaml? Or should this wait on unifying the hex values and the cmap entries? I think can be out of scope for this PR, but would be very useful to have a direct hex map here. As it stands if we add an additional clade you have to rejigger the start and stop values across different cmaps and have to do visual inspection to line up hex values with cmaps. My suggestion would be to drop cmaps and instead pad directly from hex values. This will keep things more easily in sync.
|
Another observation (that I don't know where to thread really)... if I take the resulting file |
As requested in <#67 (review)>
jameshadfield
force-pushed
the
unify-colour-defs
branch
from
10a84dc to
d8b5cfb
Compare
|
I've updated this PR to generate colour ranges from the provided clade colour (see commentary in the code of that function - I don't think the colours are better than using cmaps, but they are certainly much easier to maintain!) and also shifted the clade definitions into the config.
Oh! How annoying. |
Over time setting the clade colours & display names separately to the lineage colour definitions resulted in the two falling out of sync. This work unifies the approach so that the same script (using a unified configuration) modifies each JSON respectively. Including the colour and display name data within the clades JSON is a big improvement as it allows simple retrospective analysis of old datasets.
This avoids having to pick colour maps for each clade's colour and will help with keeping things in sync, which is clearly important for an automated forecasting pipeline. The resulting colours aren't quite as nice IMO, but the colour generation can be improved over time as needed. See <#66 (comment)> for more.
As requested in <#67 (review)>
jameshadfield
force-pushed
the
unify-colour-defs
branch
from
d8b5cfb to
dbfbb61
Compare
|
I'm going to merge this now - any colour fixes (etc) can be easily applied by subsequent PRs if needed. |
|
Thanks so much for the changes here James (and sorry that I forget to follow up). I really appreciate this. It should make things much more maintainable going forward. |
Over time setting the clade colours & display names separately to the lineage colour definitions resulted in the two falling out of sync. This work unifies the approach so that the same script (using a unified configuration) modifies each JSON respectively.
Including the colour and display name data within the clades JSON is a big improvement as it allows simple retrospective analysis of old datasets.
Tested locally with no observed changes to the resulting colours.