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

chore!: deprecation of scale param of eigen_centrality() #1527

Closed
wants to merge 12 commits into from

Conversation

maelle
Copy link
Contributor

@maelle maelle commented Sep 24, 2024

Fix #1353

But waiting on feedback there.

I also used this as an opportunity to align test files with scripts, but in separate commits. 😇

Copy link
Contributor

aviator-app bot commented Sep 24, 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 closed without merging. If you still want to merge this PR, re-open it.


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.

@maelle maelle changed the title test: fold eigen_centrality() test into test-centrality chore!: deprecation of scale param of eigen_centrality() Sep 24, 2024
@szhorvat
Copy link
Member

szhorvat commented Sep 24, 2024

To clarify, what will happen is that this parameter will be removed in C/igraph 1.0, and the function will always behave like scale=T.

A more friendly way to deal with this in R is a multi-stage deprecation:

  • In the next R/igraph version based on C/igraph 0.10, people who use scale=F should get a warning, but people who use scale=T shouldn't.
  • In the first R/igraph version based on C/igraph 1.0, people who use scale=F should get an error. People who use scale=T should get a warning.
  • In a subsequent version, scale should be removed entirely.

Maybe this is difficult and not worth the time, so it also works that any explicit use of scale should get a warning right away, and in the first R/igraph based on C/igraph 1.0, we remove scale entirely. This second option is what you seem to be going with, which is probably the best choice (saves us time).

I just wanted to be explicit about the options. I leave the choice to you :-)

@maelle maelle closed this Oct 1, 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.

Remove of scale parameter of eigen_centrality()
2 participants