Skip to content

Integrate various versions of DynApproxBetweeness code #624

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.

Sign up for GitHub

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

Jump to bottom
Merged
angriman merged 4 commits into from
Mar 16, 2022
Merged

Integrate various versions of DynApproxBetweeness code #624

angriman merged 4 commits into from
Mar 16, 2022

Conversation

@cndolo
Copy link
Collaborator

@cndolo cndolo commented Nov 13, 2020

There is some old DynApproxBetweenness that is no longer compatible with the current NetworKit version that is supposed to be integrated into NetworKit. It supports deletion for various types of graphs.

After discussing with @angriman, we decided to use the PImpl-pattern and have different implementations for the different cases. There is still a bit of long way to go before this code can be merged into the NetworKit code base.
The aim of this draft PR is to make my work and the DynApproxBetweeness code available to anyone who will takeover from here and discussions about whether using PImpl is the best solution going forward.

A brief explanation of the commits:

  • bc230e7: Converts the current DynApproxBetweennness code to pimpl pattern so that DynApproxBetweennness.h can be used with the other code.
  • e077d49: DynApproxBetweennnessImplDir - although not entirely complete, this is the first class to be ported to the current NetworKit version. There is still an unsolved error in the code + no tests. This corresponds to the class DynApproxBetweennessDir mentioned below.
  • d2be1cb: This commit just adds all the files containing the code that should be integrated in its unaltered form.

@hmeyerhenke
Copy link

hmeyerhenke commented Jun 1, 2021

Fabian, could you please look into this together with Lucas. As time permits within the next few weeks. We should not let this slip for another 6 months or so.

@avdgrinten
Copy link
Contributor

avdgrinten commented Jun 14, 2021 •
edited
Loading

This still needs a lot of work and we have to prioritize other stuff first (for the new data structure + the new release). Lucas can work on this after his other PRs are merged but it will take a few weeks. Does that sound good?

@hmeyerhenke
Copy link

hmeyerhenke commented Jun 14, 2021

It would be great if it could be part of the next release. Is that possible?

@avdgrinten
Copy link
Contributor

avdgrinten commented Jun 14, 2021

The next release is scheduled for the end of this month; this PR probably needs more than two weeks of work. We can immediately start working on it for the release though.

@hmeyerhenke
Copy link

hmeyerhenke commented Jun 14, 2021

The thing is: if it's not contained in this release, it would have to wait another six months or so -- which is exactly what I would like to avoid. Another option that I see (if the next release is impossible) is to "release" it via our research blog, which then points to the working code in the master branch.

@avdgrinten
Copy link
Contributor

avdgrinten commented Jun 14, 2021

I think the latter can be done. I would prefer not to delay the release as we have a few other interesting features already.

@fabratu
Copy link
Member

fabratu commented Aug 17, 2021 •
edited by Relux-the-Relux
Loading

Turns out there was some more code missing, which was found in older releases of NetworKit. With the latest push, an integration should be doable.

@Relux-the-Relux From my perspective the following tasks need to be done:

  • Inspect code of DynApproxBetweenessImpl, DynApproxBetweenessImplDir (both in directory centrality), DynApproxBetweeness2 and DynApproxBetweeness2W (both in directory centrality/dyn-betweenness) to see whether they are all necessary.
  • For all versions to be integrated, find more suitable class-names.
  • Port additional DynApproxBetweeness-versions to current code base (+ move out of dyn-betweenness subfolder).
  • Clean up and refactor code where necessary.
  • Rebase branch.
  • Update to comply with new networkit-format rule.
  • Inspect whether DynBFS/BFSVisit and DynDijkstra/DijkstraVisit can be merged. If so, do it. If not, find a more suitable class name for BFSVisit and DijkstraVisit.
  • Create unit tests for all DynApproxBetweeness-versions.
  • Delete unnecessary files.

@fabratu fabratu mentioned this pull request Oct 11, 2021
Closed
@Relux-the-Relux Relux-the-Relux force-pushed the dynApproxBtwn/integrate-old-code branch 8 times, most recently from 737cfd2 to 58323ee Compare December 10, 2021 13:50
angriman
angriman reviewed Dec 10, 2021
View reviewed changes
angriman
angriman reviewed Dec 10, 2021
View reviewed changes
@Relux-the-Relux Relux-the-Relux force-pushed the dynApproxBtwn/integrate-old-code branch from 803ec1e to f751794 Compare December 10, 2021 16:41
avdgrinten
avdgrinten reviewed Dec 16, 2021
View reviewed changes
Copy link
Contributor

@avdgrinten avdgrinten left a comment

Choose a reason for hiding this comment

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

Not a full review, but some important comments below.

The commit structure also needs to be revised, it is really messy.

@avdgrinten avdgrinten marked this pull request as ready for review December 16, 2021 10:30
@Relux-the-Relux Relux-the-Relux marked this pull request as draft January 3, 2022 15:47
@Relux-the-Relux Relux-the-Relux force-pushed the dynApproxBtwn/integrate-old-code branch 2 times, most recently from 9a98e4b to 1eb6005 Compare January 10, 2022 14:34
@Relux-the-Relux Relux-the-Relux marked this pull request as ready for review January 10, 2022 14:35
@Relux-the-Relux Relux-the-Relux force-pushed the dynApproxBtwn/integrate-old-code branch from 1eb6005 to 50e15b9 Compare January 10, 2022 15:40
angriman
angriman reviewed Jan 11, 2022
View reviewed changes
angriman
angriman reviewed Jan 11, 2022
View reviewed changes
angriman
angriman reviewed Feb 18, 2022
View reviewed changes
angriman
angriman reviewed Feb 18, 2022
View reviewed changes
@Relux-the-Relux Relux-the-Relux force-pushed the dynApproxBtwn/integrate-old-code branch from b60f746 to 03bd930 Compare February 18, 2022 15:25
angriman
angriman reviewed Feb 18, 2022
View reviewed changes
@Relux-the-Relux Relux-the-Relux force-pushed the dynApproxBtwn/integrate-old-code branch from 03bd930 to fe60cd5 Compare February 21, 2022 12:47
angriman
angriman reviewed Feb 21, 2022
View reviewed changes
@Relux-the-Relux Relux-the-Relux force-pushed the dynApproxBtwn/integrate-old-code branch 5 times, most recently from 6c064b5 to d6f15ea Compare February 23, 2022 15:26
angriman
angriman reviewed Feb 23, 2022
View reviewed changes
@Relux-the-Relux Relux-the-Relux force-pushed the dynApproxBtwn/integrate-old-code branch 2 times, most recently from 2459143 to 2243c24 Compare February 25, 2022 10:08
@Relux-the-Relux Relux-the-Relux force-pushed the dynApproxBtwn/integrate-old-code branch from 2243c24 to 0699e17 Compare March 4, 2022 10:49
angriman
angriman reviewed Mar 4, 2022
View reviewed changes
@Relux-the-Relux Relux-the-Relux force-pushed the dynApproxBtwn/integrate-old-code branch from 0699e17 to a0474d8 Compare March 4, 2022 16:59
angriman
angriman reviewed Mar 4, 2022
View reviewed changes
angriman
angriman reviewed Mar 4, 2022
View reviewed changes
angriman
angriman reviewed Mar 4, 2022
View reviewed changes
angriman
angriman reviewed Mar 4, 2022
View reviewed changes
angriman
angriman reviewed Mar 4, 2022
View reviewed changes
angriman
angriman reviewed Mar 4, 2022
View reviewed changes
@Relux-the-Relux Relux-the-Relux force-pushed the dynApproxBtwn/integrate-old-code branch from a0474d8 to 536a6d4 Compare March 7, 2022 12:00
@angriman angriman merged commit 03fbb35 into networkit:master Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@avdgrinten avdgrinten avdgrinten left review comments

@angriman angriman angriman approved these changes

+1 more reviewer

@Relux-the-Relux Relux-the-Relux Relux-the-Relux left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@fabratu fabratu

@Relux-the-Relux Relux-the-Relux

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@cndolo @hmeyerhenke @avdgrinten @fabratu @angriman @Relux-the-Relux