Skip to content

feat: weight support for eccentricity() and radius()#1211

Merged
maelle merged 12 commits intomainfrom
feat/weighted-eccenctricity
Mar 25, 2024
Merged

feat: weight support for eccentricity() and radius()#1211
maelle merged 12 commits intomainfrom
feat/weighted-eccenctricity

Conversation

@szhorvat
Copy link
Copy Markdown
Member

@szhorvat szhorvat commented Feb 6, 2024

Fixes #893

@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Feb 6, 2024

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

@szhorvat

This comment was marked as outdated.

@szhorvat szhorvat force-pushed the feat/weighted-eccenctricity branch from 686efe8 to da90c2f Compare February 6, 2024 19:43
@szhorvat

This comment was marked as outdated.

@szhorvat szhorvat force-pushed the feat/weighted-eccenctricity branch from da90c2f to 84213ac Compare February 6, 2024 19:47
@maelle

This comment was marked as outdated.

@szhorvat

This comment was marked as outdated.

@szhorvat

This comment was marked as outdated.

@maelle maelle requested a review from krlmlr March 4, 2024 16:59
Copy link
Copy Markdown
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks.

  • Is the order of functions in functions-R.yaml consistent with that of functions.yaml ? We probably want a simple test for that.
  • Can you please add tests for the new functions?

I'm fine with the PR the way it is, we can always refine later.

@krlmlr krlmlr added this to the 2.0.3 milestone Mar 12, 2024
@krlmlr

This comment was marked as outdated.

@szhorvat

This comment was marked as outdated.

@maelle

This comment was marked as outdated.

@krlmlr krlmlr modified the milestones: 2.0.3, triage Mar 13, 2024
@maelle maelle self-assigned this Mar 18, 2024
@maelle maelle force-pushed the feat/weighted-eccenctricity branch from a93e1b0 to c60f7cd Compare March 18, 2024 14:06
@maelle
Copy link
Copy Markdown
Contributor

maelle commented Mar 18, 2024

  • I added the lifecycle infrastructure. Does this seem correct @krlmlr?
  • I added test cases. Do they make any sense @szhorvat? If so it would make sense to add the same test cases as examples especially for graph_center() that currently has no example.

Copy link
Copy Markdown
Contributor

@krlmlr krlmlr 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, thanks!

@szhorvat
Copy link
Copy Markdown
Member Author

szhorvat commented Mar 19, 2024

  • I added test cases. Do they make any sense @szhorvat? If so it would make sense to add the same test cases as examples especially for graph_center() that currently has no example.

I had a very quick look, seems fine as a smoke test, and yes, the test you added for graph_center() would actually make a very good example (perhaps with a smaller tree).

Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
@maelle maelle marked this pull request as ready for review March 22, 2024 10:40
@maelle
Copy link
Copy Markdown
Contributor

maelle commented Mar 22, 2024

@szhorvat could you please fix the conflicts in src/cpp11.cpp, or tell me how to fix them? Thank you!

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Mar 22, 2024

cpp11::cpp_register()

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Mar 22, 2024

Needs the decor package.

@szhorvat
Copy link
Copy Markdown
Member Author

szhorvat commented Mar 22, 2024

I merged main into this branch and fixed the conflict by regenerating this file using R -q -e 'cpp11::cpp_register()'

Does this mess up Aviator because it won't be able to rebase anymore?

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Mar 22, 2024

Aviator might be fine, I can't identify the reason why it sometimes fails to rebase.

@maelle
Copy link
Copy Markdown
Contributor

maelle commented Mar 25, 2024

thanks both!

@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Mar 25, 2024

This pull request failed to merge: PR cannot be automatically rebased, please rebase manually to continue. Remove the blocked label to re-queue.

Additional debug info: Failed to rebase this PR onto the latest changes from the base branch. You will probably need to rebase this PR manually and resolve conflicts).

@maelle maelle merged commit aa0de48 into main Mar 25, 2024
@maelle maelle deleted the feat/weighted-eccenctricity branch March 25, 2024 13:59
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 26, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Could the functions eccentricity() and radius() be implemented for weighted graphs?

3 participants