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

Test suite fails when using "igraph" versions >= 1.4.0 #236

Closed
bockthom opened this issue Mar 18, 2023 · 1 comment
Closed

Test suite fails when using "igraph" versions >= 1.4.0 #236

bockthom opened this issue Mar 18, 2023 · 1 comment

Comments

@bockthom
Copy link
Collaborator

In igraph version 1.4.0, they have introduced a breaking change regarding attribute types (see here: https://github.com/igraph/rigraph/blob/main/NEWS.md#igraph-140). This breaking change in igraph 1.4.0 breaks our test suite seriously, hundreds of tests fail.

igraph 1.4.0 was released on 22nd of February 2023. There are multiple reasons how the recently introduced breaking changes of igraph 1.4.0 affect our test suite. For the moment, I just looked at one failing test and spotted two of these reasons, but there can also be more than these two, of course:


(1) Using the AsIs class to generate the expected test output lets tests fail

igraph now respects all types and classes (which have been ignored before). That is, creating data.frames containing I(...) calls and creating networks from the data.frame via graph.data.frame now creates networks that are different than the networks coronet creates, due the AsIs class introduced by I(...) which has been ignored in previous igraph versions.
However, this can be easily fixed by calling unclass for each data.frame column that has been created via I(...).

Example: Consider the following test for author networks which uses I(...) to create the expected data.frame.

data = data.frame(
from = c("Björn", "Olaf", "Olaf", "Karl"),
to = c("Olaf", "Karl", "Thomas", "Thomas"),
date = I(list(c(1468339139, 1468339245), c(1468339541, 1468339570), c(1468339541, 1468339592),
c(1468339570, 1468339592))),
artifact.type = I(list(c("Feature", "Feature"), c("Feature", "Feature"), c("Feature", "Feature"),
c("Feature", "Feature"))),
hash = I(list(
c("72c8dd25d3dd6d18f46e2b26a5f5b1e2e8dc28d0", "5a5ec9675e98187e1e92561e1888aa6f04faa338"),
c("3a0ed78458b3976243db6829f63eba3eead26774", "1143db502761379c2bfcecc2007fc34282e7ee61"),
c("3a0ed78458b3976243db6829f63eba3eead26774", "0a1a5c523d835459c42f33e863623138555e2526"),
c("1143db502761379c2bfcecc2007fc34282e7ee61", "0a1a5c523d835459c42f33e863623138555e2526"))),
file = I(list(c("test.c", "test.c"), c("test2.c", "test3.c"), c("test2.c", "test2.c"), c("test3.c", "test2.c"))),
artifact = I(list(c("A", "A"), c("Base_Feature", "Base_Feature"), c("Base_Feature", "Base_Feature"),
c("Base_Feature", "Base_Feature"))),
weight = 2,
type = TYPE.EDGES.INTRA,
relation = "cochange"
)

For each of the columns that uses I(...) we just need to call unclass afterwards. So, in the example above, we need to add the following lines below the data.frame creation:

 data[["date"]] = unclass(data[["date"]])
 data[["artifact.type"]] = unclass(data[["artifact.type"]])
 data[["hash"]] = unclass(data[["hash"]])
 data[["file"]] = unclass(data[["file"]])
 data[["artifact"]] = unclass(data[["artifact"]])

This fix does not affect previous versions of igraph. Hence, for all igraph versions, this problem can be fixed by just adding these unclass calls where applicable.


(2) igraph now respects types in edge attributes

In previous versions of igraph, types of newly set edge attributes where ignored. For example, the date attribute was introduced as an attribute of type "numeric". Even when the actual attribute was set as an "POSIXct", the date attribute was still stored as being of type "numeric". However, now the types are respected and dates are stored as "POSIXct". In most of our tests, this is not a problem, as the created data.frame for the expected network already correctly converts the dates to "POSIXct" - so our expected data is correct and the tests formerly worked though because this type was just ignored. However, there are tests which have a weird behavior:

Let's have a look at the author-network creation test already discussed above:
Example: Consider the following test for author networks which uses I(...) to create the expected data.frame.

date = I(list(c(1468339139, 1468339245), c(1468339541, 1468339570), c(1468339541, 1468339592),
c(1468339570, 1468339592))),

For whatever reason, in this test, unix timestamps are used instead of dates in string format. Therefore, this test fails with igraph >= 1.4.0, as unix timestamps are of type "numeric" and not of type "POSIXct". This is easily fixable - either by calling get.date.from.unix.timestamp or by using the string representation of the date, as in other tests. However, with this fix, the test does not work any more for igraph versions < 1.4.0. So, we need a solution here that works for both recent and older igraph versions.
@hechtlC As you have authored this test, do you remember why you have used the unix timestamps here?
We need to find out why this test behaves different than all the other tests - either igraph did (and still does) some inconsistent type processing, or we somehow do some inconsistent processing in coronet.


This issue affects all versions of coronet from version 3.1 on up until now (i.e., version 4.2).

@bockthom
Copy link
Collaborator Author

bockthom commented Apr 2, 2023

Short summary: @hechtlC and I had decided to fix the test in coronet/tests/test-networks-author.R (see comment above) in such a way that we extract the class of the date attribute from the built network and choose the right date conversion function for this single test afterwards accordingly. This way, we make sure that this test passes independent of the used igraph version (i.e., igraph < 1.4.0 or igraph >= 1.4.0).


In addition to the above described problems, a couple of other problems arose in relation to the new attribute class handling as of igraph version 1.4.0. I have already implemented some fixes for them.
For all the problems that arose in relation to this breaking change of igraph, I will open a pull request soon that fixes all the related problems. See the corresponding pull request for more details.

bockthom added a commit to bockthom/coronet that referenced this issue Apr 2, 2023
As of igraph version 1.4.0, the classes of edge attributes are respected in
igraph. Therefore, dates in the expected outcomes of our tests need to be of
class `POSIXct`. As the `I()` operator prevents an automatic conversion, we need
to manually convert the dates to the correct class. However, as `I()` adds the
`AsIs` class, we need to remove this class from the affected edge attributes
again, as otherwise the expected network and the built network would not be
equal.

To make this test succeed in both, igraph versions < 1.4.0 and igraph versions
>= 1.4.0, the expected class is derived from the class of the date attribute in
the built network. This way, the test is able to pass independent of the used
igraph version.

This fix addresses se-sic#236.

Signed-off-by: Christian Hechtl <hechtl@cs.uni-saarland.de>
Signed-off-by: Thomas Bock <bockthom@cs.uni-saarland.de>
bockthom added a commit to bockthom/coronet that referenced this issue Apr 2, 2023
As of igraph version 1.4.0, igraph respects the classes of edge and vertex
attributes. Therefore, we have to make sure that we add the correct classes when
we add new edge or vertex attributes to a (possibly empty) network.

To do so, in function `add.attributes.to.network`, we need to set the
`default.value` (for which we have already set the correct class before) instead
of `NA` (which is interpreted as `logical` if no concrete class is given).

In addition, if the attribute is of class `POSIXct`, an additional time zone
attribute is necessary. We use the default time zone that is set in
`util-init.R` in this case.

This fix addresses se-sic#236.

Signed-off-by: Thomas Bock <bockthom@cs.uni-saarland.de>
bockthom added a commit to bockthom/coronet that referenced this issue Apr 2, 2023
When edge or vertex attributes are added via `add.attributes.to.network`, as of
igraph version 1.4.0, existing attributes are not re-added or re-ordered. That
is, if attributes `name` and `type` already exist and ones adds the new
attributes `kind` and `type`, then `kind` is added after `type` as `type`
already existed. However, this breaks the ordering of the attributes and leads
to failing tests, as the order of the attributes is also tested in some of our
tests. To fix this problem, remove and re-add an attribute that was already
present before. This way, we make sure that the new attributes are added in the
expected order. (The only exception is the `name` attribute: This attribute is
expected to be the first attribute, and it should not be removed and readded as
this could lead to serious problems.)

This addresses se-sic#236.

Signed-off-by: Thomas Bock <bockthom@cs.uni-saarland.de>
bockthom added a commit to bockthom/coronet that referenced this issue Apr 2, 2023
…n igraph)

As of igraph version 1.4.0, igraph respects the classes of edge and vertex
attributes. However, the function `igraph::disjoint_union` does not respect
their classes and converts `POSIXct` attributes to numeric attributes, for
instance. This is a bug in igraph that is expected to be fixed in later igraph
versions. For now, we temporarily fix this problem on our own by converting the
`date` attribute of the union back to `POSIXct`.
Note: Also other attributes could be affected! However, for convenience reasons,
we only apply our temporary fix to the `date` attribute.

This addresses se-sic#236.

Signed-off-by: Thomas Bock <bockthom@cs.uni-saarland.de>
bockthom added a commit to bockthom/coronet that referenced this issue Apr 2, 2023
The breaking changes of igraph version 1.4.0 regarding the classes of edge
attributes revealed an actual bug in the edge attributes of bipartite edges in
coronet:

We need to make sure that we only add the edge attributes for the edges that are
really added to the network. As, for example, edges to the untracked artifact
are not added to our bipartite network, we also need to make sure to remove the
attributes of the corresponding edges (otherwise this could lead to wrong
attributes). Therefore, we add a (rather complex) check for which edges (i.e.,
vertex sequences) are present in the final bipartite network and extract the
corresponding information for the edge attributes accordingly.

This addresses se-sic#236.

Signed-off-by: Thomas Bock <bockthom@cs.uni-saarland.de>
bockthom added a commit to bockthom/coronet that referenced this issue Apr 2, 2023
…n igraph)

As of igraph version 1.4.0, igraph respects the classes of edge and vertex
attributes. However, the function `igraph::disjoint_union` does not respect
their classes and converts `POSIXct` attributes to numeric attributes, for
instance. This is a bug in igraph that is expected to be fixed in later igraph
versions. For now, we temporarily fix this problem on our own by converting the
`date` attribute of the union back to `POSIXct`.
Note: Also other attributes could be affected! However, for convenience reasons,
we only apply our temporary fix to the `date` attribute.

This addresses se-sic#236.

Signed-off-by: Thomas Bock <bockthom@cs.uni-saarland.de>
bockthom added a commit to bockthom/coronet that referenced this issue Apr 2, 2023
The breaking changes of igraph version 1.4.0 regarding the classes of edge
attributes revealed an actual bug in the edge attributes of bipartite edges in
coronet:

We need to make sure that we only add the edge attributes for the edges that are
really added to the network. As, for example, edges to the untracked artifact
are not added to our bipartite network, we also need to make sure to remove the
attributes of the corresponding edges (otherwise this could lead to wrong
attributes). Therefore, we add a (rather complex) check for which edges (i.e.,
vertex sequences) are present in the final bipartite network and extract the
corresponding information for the edge attributes accordingly.

This addresses se-sic#236.

Signed-off-by: Thomas Bock <bockthom@cs.uni-saarland.de>
@hechtlC hechtlC closed this as completed Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants