Skip to content

Tests and Fixes for utils-networks-misc.R#248

Merged
hechtlC merged 18 commits intose-sic:devfrom
Leo-Send:pullrequest
Jan 25, 2024
Merged

Tests and Fixes for utils-networks-misc.R#248
hechtlC merged 18 commits intose-sic:devfrom
Leo-Send:pullrequest

Conversation

@Leo-Send
Copy link
Contributor

@Leo-Send Leo-Send commented Jan 9, 2024

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

This Pull request fixes #242

Changelog

Added

Changed/Improved

Fixed

@Leo-Send Leo-Send marked this pull request as draft January 9, 2024 15:43
For testing purposes

Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
to work if author list does not contain all authors from network
and print a warning if that is the case

Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
to have correct column- and rownames in the returned matrices

Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
to give a warning if the matrices have differing column- or rownames.
Also changed the documentation to reflect this

Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
convert.adjacency.matrix.list.to.array now
throws an error when used with empty list

Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
@Leo-Send Leo-Send marked this pull request as ready for review January 17, 2024 13:04
Now named assert.sparse.matrices.equal and  assert.networks.equal

Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
Copy link
Contributor

@hechtlC hechtlC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request looks good. Thanks for the new tests and the fixes @Leo-Send !
I have just found some small issues that need to be fixed. After they have been fixed and @bockthom is fine with the changes, we can merge the PR.

Copy link
Collaborator

@bockthom bockthom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me @Leo-Send, I only found "a few" indentation or formatting issues 😉
I am not sure if you use tabs or spaces during implementation - it might be that the wrong indentation comes from using tabs instead of spaces - if so, please configure your IDE in such a way that it does not use tabs.

And there is one line where the code should be made fail save (but nothing critical, so no need to adjust the NEWS), and sometimes an additional comment would be helpful. See my detailed comments below.

Signed-off by: Leo Sendelbach <s8lesend@stud.uni-saarland.de>
@hechtlC hechtlC merged commit 603a4f2 into se-sic:dev Jan 25, 2024
@bockthom bockthom mentioned this pull request Apr 16, 2024
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.

3 participants