Correctly handle date attributes in list format in 'get.edgelist.with.timestamps'

[maxloeffler]

Prerequisites

• I adhere to the coding conventions (described here) in my code.
• I have updated the copyright headers of the files I have modified.
• I have written appropriate commit messages, i.e., I have recorded the goal, the need, the needed changes, and the location of my code modifications for each commit. This includes also, e.g., referencing to relevant issues.
• I have put signed-off tags in all commits.
• I have updated the changelog file NEWS.md appropriately.
• I have checked whether I need to adjust the showcase file showcase.R with respect to my changes.
• The pull request is opened against the branch dev.

Description

In #274, we changed the default representation of edge attributes from vectors to lists. During this transition, the get.edgelist.with.timestamps method was updated properly. Here, we update the method to work correctly with dates in either vector or list format. Also, we add a new unlist.timestamps.if.possible parameter to the method to allow callers to assert that timestamps in the return value of get.edgelist.with.timestamps are vectors instead of lists

Changelog

Added

• Add unlist.timestamps.if.possible parameter to get.edgelist.with.timestamps to allow callers to receive timestamps in vector format instead of list format (when possible)

Fixed

• Handle date edge attributes in list format correctly in get.edgelist.with.timestamps

[bockthom]

Thanks for this PR @maxloeffler. I did not spot anything to criticize and would directly approve it.

However, seems like you have been too slow 🙈 🙃 The test suite fails because of some errors in the plotting functionality (which is completely unrelated to this PR). Unfortunately, the plotting library we use (ggplot2) has published a major release (4.0.0) a couple of days ago. I've checked their NEWS for breaking changes but did not find anything that directly matches the error that occurs in our test suite. But some of their changes must affect the way we use it. Could you please debug this and figure out how to fix this? Preferably we would like to find a solution that works with the new version of ggplot2 and previous versions... but this depends on what the actual problem is here...

Sorry for the additional effort, but you are the perfect guy for dealing with such issues – you have already lots of experience with stuff like that 🥲

[maxloeffler]

No problem ^^ I've looked into the problem and it's actually quite interesting. Disclaimer: While I figured out where the problem lies and could fix it for older and newer ggplot2 versions (I tested 3.4.4 and 4.0.0) I did not precisely figure out why it broke just now.

Run down

So in plotting there is this function plot.get.plot.for.network in which we define the style of all elements in the final plots, so the different vertices and edges and so on. Previously, we styled the edges using this call:

ggraph::scale_edge_linetype(name = "Relation Types")

This ggraph function internally creates a discrete scale (mapping between edge types and resulting styles if I understand it correctly). Here is the code:

scale_edge_linetype <- function(..., na.value = 'blank') {
  sc <- scale_linetype(..., na.value = na.value)
  sc$aesthetics <- 'edge_linetype'
  sc
}

scale_linetype is a function in ggplot2. Here is its code:

scale_linetype <- function(name = waiver(), ..., aesthetics = "linetype") {
  discrete_scale(
    aesthetics, name = name,
    palette = NULL,
    ...
  )
}

Problem and Fix

As you can see, the final call to ggplot2::discrete_scale sets palette to NULL. This causes the error we observe because NULL is not a valid palette. First I tried passing a palette to the ggraph::scale_edge_linetype call we previously had. However, this does not work since then the two explicit definitions of palette clash. The fix for this problem is to call the bottom most ggplot2::discrete_scale ourselves with exactly the parameters we want:

ggplot2::discrete_scale(name = "Relation Types", aesthetics = "edge_linetype", palette = scales::pal_linetype())

For this call, I defaulted to using the standard palette that scales provides for linetypes (edges). From looking at the plots in showcase.R, I can pretty much not spot a difference so I assume that this was the default palette before anyways. However, if you want to, we can always use other palettes here.

[bockthom]

I did not precisely figure out why it broke just now.

I did: This commit introduced palette = NULL, which is only tagged with release 4.0.0 of ggplot2 - so that's the reason why it appeared now - there was no earlier release that contained this commit:
tidyverse/ggplot2@1bb9230

I've checked the issues of ggplot2, and there have been already 8 new issues created since their release of version 4.0.0. Seems that they have messed up quite a number of things in their release... But they did not react to any of these 8 issues yet...

Ultimate Solution

Thank you very much for coming up with a fix in a relatively short period of time, good work! While this fix might potentially work as a temporary fix, I don't think we should generally fix ggplot2 internals such as passing "edge_linetype" on our end. So, the question is: Can we create an issue and report this problem - it seems not to be intended behavior. If so, is this something ggraph needs to fix in order to be compatible with the new version of ggplot2, or do you think this is something that primarily affects ggplot2 itself and should be reported there? (I'd tend toward the latter, but actually have no idea...) I'd appreciate it if you could prepare an issue report (if you agree that this is something we should report).

Temporary fix

Not sure how fast ggplot2 will come up with a bugfix release - but given that they have not reacted to any of the 8 new issues yet, I think it might take quite a while until they will eventually fix and release it... if they do it at all... So, I'd suggest to go with your temporary fix (keeping the previous code as a comment, and adding a comment pointing to this discussion). If the color palette is exactly the same as before, everything should be fine. As the ggplot2 release might also affect other scripts that use coronet, I aim at running all the other scripts with the new ggplot2 version some time next week, to see if other functions break as well.

[maxloeffler]

I would assume that this is a problem with ggplot2 not ggraph. The palette is being set to NULL in ggplot2::scale_linetype though the documentation does not provide any explanation or reason for it and the error is being thrown from inside ggplot2 as well.

What I do not know is what the reason behind the NULL value is, i.e., whether there was thought behind it. Therefore and due to my lack of familiarity for the ggplot2 ecosystem I find it hard to formulate a precise PR.

I'll follow your suggestion to integrate the temporary fix for now and see about opening a PR soon.

[maxloeffler]

(Side note: I updated the Add 'unlist.timestamps.if.possible' parameter to convert dates to vector commit because there was a missing parenthesis at the end of the newly introduced section of the tests that broke syntactic correctness, other than that nothing changed)

[codecov]

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.57%. Comparing base (88356e1) to head (9ae2a8e).
⚠️ Report is 5 commits behind head on dev.

Files with missing lines	Patch %	Lines
util-plot.R	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #289      +/-   ##
==========================================
+ Coverage   82.55%   82.57%   +0.01%     
==========================================
  Files          16       16              
  Lines        5378     5378              
==========================================
+ Hits         4440     4441       +1     
+ Misses        938      937       -1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bockthom]

What I do not know is what the reason behind the NULL value is, i.e., whether there was thought behind it. Therefore and due to my lack of familiarity for the ggplot2 ecosystem I find it hard to formulate a precise PR.

I'll follow your suggestion to integrate the temporary fix for now and see about opening a PR soon.

No worries, I also don't understand what the reason behind the NULL value is neither, and you also don't need to open a PR. I just think that is a problem that should be reported to ggplot2 in an issue (not a PR), to make them aware of the problem. Regarding the reason behind the NULL value, you might have a look at the commit that I have linked above in an earlier comment.

In addition, could you please update your copyright header in util-plot.R? Thanks! (We can ignore codecov's comment here as we don't have tests for the plotting module at all.)

[Copilot]

Pull Request Overview

This PR fixes handling of date attributes in list format for the get.edgelist.with.timestamps function and adds an optional parameter to allow unlisting timestamps when possible.

• Updates get.edgelist.with.timestamps to correctly handle date attributes in both vector and list formats
• Adds unlist.timestamps.if.possible parameter to control timestamp format in return value
• Includes comprehensive test coverage for the new functionality

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
util-plot.R	Workaround for bug in ggraph scale functions by using ggplot2 directly
util-misc.R	Enhanced get.edgelist.with.timestamps with new parameter and list handling logic
tests/test-misc.R	Added comprehensive test coverage for both vector and list date formats

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[bockthom]

Looks good now. The analysis script that previously has thrown an error in get.edgelist.with.timestamps works now again.

Could you please update the NEWS now? We should add an item for the fix there if this bug was already caused by changes of the previous release of coronet. If the bug was not present in the last release, just add the commit hash to the item that describes the changes in which we have wrapped the dates in lists without further description. Adding a new parameter to the function, however, may need a separate entry below the one which is related to first commit.
Regarding the plotting fix for ggplot2: As this is not a fix in the functionality of coronet but of external libraries, I am not sure whether we need a NEWS entry for that. There has already been a similar workaround applied to the same plotting function earlier, could you please figure out whether we had a NEWS entry for this former workaround or not? I'd like to do it in a similar way as for the previous fix caused by the same library.

And last but not least: As already mentioned above, could you please draft a bug report for the issue that you have figured out in ggplot2, including a minimum example to reproduce the bug? I'd like to discuss the draft of the bug report prior to submitting it to ggplot2.

[maxloeffler]

Dates are lists since release 5.0 so this change must receive a "fix entry". The other workaround you mentioned in plotting is an interesting case. It was introduced 7 years ago and was seemingly never anything more than a comment that suggests on how to do it "correctly". Therefore, it also does not receive any explicit mention in the NEWS which I think is also correct because this is not a change that must be externally communicated.

[maxloeffler]

Drafted Bug Report

Hey ggplot team,

I stumbled upon an error in the scale_linetype function and its internal invocation of discrete_scale. When calling the function with aesthetic = edge_linetype, I observe the following error (which I suspect to be related to palette being NULL in this case):

Error in `as_discrete_pal()`:
! Cannot convert `x` to a discrete palette.

Note, however, that the error does not occur if aesthetics is omitted or changed, e.g., to linetype. Furthermore, it does not appear when we manually call discrete_scale with aesthetic = edge_linetype as soon as we provide a palette. Here is minimal code to reproduce the bug:

library(igraph)
library(ggraph)
library(ggplot2)
library(scales)

network = igraph::make_graph(c("A", "B"))

# works
plot = ggraph::ggraph(network)
plot = plot + ggplot2::scale_linetype(aesthetics = "linetype")
print(plot)

# works
plot = ggraph::ggraph(network)
plot = plot + ggplot2::scale_linetype()
print(plot)

# works
plot = ggraph::ggraph(network)
plot = plot + ggplot2::discrete_scale(aesthetics = "edge_linetype", palette = scales::pal_linetype())
print(plot)

# breaks
plot = ggraph::ggraph(network)
plot = plot + ggplot2::scale_linetype(aesthetics = "edge_linetype")
print(plot)

[bockthom]

@maxloeffler Thanks for all the updates, and also thanks a lot for drafting the bug report (which we will discuss in our meeting).

I can approve this PR now and I will certainly merge it right after our discussion of the bug-report draft for the ggplot2 bug.

[maxloeffler]

@bockthom I overhauled the drafted bug report. I mentioned the discussed case in text and included all properly working cases in code as well

[bockthom]

Thanks @maxloeffler for updating the drafted bug report! I will report it to ggplot2 right away.