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

feat!: as_adjacency_matrix() no longer supports attributes of type character #1072

Merged
merged 3 commits into from Jan 9, 2024

Conversation

krlmlr
Copy link
Contributor

@krlmlr krlmlr commented Jan 2, 2024

For #907.

I see how it is useful to advance on that front. Running revdepchecks, if they succeed, I'll merge. Anything beyond that will have to wait until after 2.0.0.

Copy link
Contributor

aviator-app bot commented Jan 2, 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 using Aviator.


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.

@krlmlr krlmlr requested a review from szhorvat January 2, 2024 21:32
@krlmlr krlmlr force-pushed the f-907-adj-chr branch 2 times, most recently from 8641014 to f01015a Compare January 3, 2024 15:03
@krlmlr krlmlr added this to the upgrade-2 milestone Jan 4, 2024
@krlmlr
Copy link
Contributor Author

krlmlr commented Jan 4, 2024

Two packages have new breakages, I believe SCORPIUS is a false positive, but checking again.

There are two paths from here:

  • We stop support for this functionality, and ask downstream to change their implementation
  • We add a test for this functionality and keep supporting it

No urgency from my end.

@krlmlr krlmlr marked this pull request as draft January 4, 2024 07:19
@szhorvat
Copy link
Member

szhorvat commented Jan 5, 2024

It makes sense to stop support, as this can't be implemented in the C core, and its usefulness is dubious. Eventually all adjacency matrix stuff should rely on the C implementation.

That said, it would be fair to contact these packages and see why they are using this functionality, and whether it is easy for them to stop relying on it. I don't see any good reason for working with string-based adjacency matrices, but maybe I am missing something.

@krlmlr
Copy link
Contributor Author

krlmlr commented Jan 6, 2024

@sarahleavitt @schochastics @robertjankowski: The nbTransmission and signnet packages are using as_adjacency_matrix() with an edge attribute of type character. Is this on purpose, or perhaps a mistake? Can you work around so that your packages continue working with this PR?

Install with pak::pak("igraph/rigraph@f-907-adj-chr") .

@schochastics
Copy link

Just checked my usecase and I can work around this without issues. I will fix it in the next few days

@schochastics
Copy link

@krlmlr already fixed it. I will submit signnet 1.0.4 when CRAN submissions are online again

@szhorvat
Copy link
Member

szhorvat commented Jan 6, 2024

@schochastics I am interested in what your use case was an how you worked around it. Can you share it? Are you working with complex-values adjacency matrices, or complex-values attributes? Is this something that makes sense to support in C/igraph in the future? I'm primarily interested in the theory behind this, not the implementation.

@schochastics
Copy link

@szhorvat Sure! The technical details are in this paper.
When you project signed two mode networks, a third type of tie can occur (called the "ambivalent tie"). To support it in signnet, I use an edge attribute type with values "N", "P", and "A" and those get transformed to a complex adjacency matrix with P=1+0i, N=0+1i and A=0.5+0.5i. It was convenient to build it with as_adj(g, attr="type") and then convert "P" "N" and "A" entries to complex values, but it was trivial to work around this.

I would like to keep the "N", "P", and "A" nomenclature, because it is easier to understand for users and it is enough for most of the functionality of signnet. The complex matrix part is still very experimental but I think there are some other papers out there that work with complex adjacency matrices (in different context), so maybe it is something to support in future versions.

@sarahleavitt
Copy link

sarahleavitt commented Jan 9, 2024 via email

@krlmlr krlmlr modified the milestones: upgrade-2, upgrade Jan 9, 2024
@krlmlr krlmlr marked this pull request as ready for review January 9, 2024 21:12
@aviator-app aviator-app bot merged commit 187eb75 into main Jan 9, 2024
14 checks passed
@aviator-app aviator-app bot deleted the f-907-adj-chr branch January 9, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants