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

Miscellaneous improvements regarding network splitting and simplification #268

Merged
merged 8 commits into from
Sep 12, 2024

Conversation

maxloeffler
Copy link
Contributor

@maxloeffler maxloeffler commented Aug 27, 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 PR Includes somewhat minor fixes for multiple open issues.

Changelog

  • Replace igraph::is.loop by igraph::which_loop and igraph::list.graph.attributes by igraph::graph_attr_names
  • When splitting by bins, disregard all elements that occur before the first specified bin
  • Add 'remove.duplicate.edges' function that disambiguates edges by the exact identity relation (and corresponding test)
  • Add cumulative parameter to the construct.ranges function which allows to create cumulative ranges from the given revisions

Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 80.92%. Comparing base (55dc0cc) to head (7c8b8f8).
Report is 9 commits behind head on dev.

Files with missing lines Patch % Lines
util-plot.R 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #268      +/-   ##
==========================================
+ Coverage   80.89%   80.92%   +0.02%     
==========================================
  Files          16       16              
  Lines        5036     5043       +7     
==========================================
+ Hits         4074     4081       +7     
  Misses        962      962              

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

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.

Thanks! This PR looks good to me. I just have a few comments regarding documentation and tests.

This change is in-line with the current treatment of the elements that
occur after the last bin.

Further, this fixes a minor "bug" where bin indecees are used to
determine bin labels by indexing into a list of labels. Elements before
the first bin received bin index 0, however, R is 1-indexed. Note: This
"bug" did not cause problems because the bin with the incorrect label
will be ignored later anyways.

Works towards se-sic#267.

Signed-off-by: Maximilian Löffler <[email protected]>
Add a function that takes a network as input and simply removes all
edges that are exact duplicates of each other.

Works towards se-sic#138.

Signed-off-by: Maximilian Löffler <[email protected]>
@bockthom
Copy link
Collaborator

Just one final question before I can approve this PR: The splitting fix regarding the elements before the first bin does not have any user-visible effects? If this really is the case, I am fine with not mentioning it. If it can have user-visible effects, we should mention it in the NEWS.md.

@maxloeffler
Copy link
Contributor Author

Yup, it has no user-visible effect. That is why I found it hard to justify mentioning it in the NEWS and decided against it 😄

@bockthom bockthom merged commit 34137e8 into se-sic:dev Sep 12, 2024
9 checks passed
@bockthom bockthom mentioned this pull request Feb 2, 2025
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.

2 participants