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!: Stop accepting setting layout as a graph attribute if it is a matrix #1303

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

krlmlr
Copy link
Contributor

@krlmlr krlmlr commented Mar 12, 2024

Closes #775.

@maelle: Can you please pick up from here?

Copy link
Contributor

aviator-app bot commented Mar 12, 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 pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.


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 marked this pull request as draft March 12, 2024 14:50
Copy link
Contributor

aviator-app bot commented Mar 12, 2024

This pull request failed to merge: this PR is in draft state. Remove the blocked label to re-queue.

Additional debug info: PR was marked as draft after queueing

@aviator-app aviator-app bot added blocked and removed mergequeue labels Mar 12, 2024
Copy link
Contributor

aviator-app bot commented Mar 12, 2024

This pull request can't be queued because it's currently a draft.

@szhorvat
Copy link
Member

Then there will need to be a convenient way to enter the layout information as vertex attributes (x and y, or x, y and z).

I'm not exactly sure what features exist for this.

There's e.g. add_layout_() which actually adds a graph attribute ...

@krlmlr
Copy link
Contributor Author

krlmlr commented Mar 12, 2024

Accept a vertex attribute layout that is decomposed into x, y and z, and return a layout attribute when asked and if x and y exist?

@szhorvat
Copy link
Member

Something of that sort, yes.

The problem is that the function that compute layouts return matrices.

But in order to store layouts in graphs properly, we need to split these matrices into x, y and maybe z components.

For convenience, there should be some easy way to convert between the two representations. In particular, there should be a very easy way to compute a layout (produced as a matrix) and store it in the graph as x, y vertex attributes. Perhaps the add_layout_() function should be updated to do precisely this. This is a "mildly breaking" change.

(If you ask me, the best solution would be if coordinate tuples could be stored in a single "position" vertex attribute, but unfortunately the C core inly supports scalars at the moment, not vectors/tuples, as attribute values associated with individual vertices. I hope we get the new grant from the CZI and can work on improving this.)

@maelle maelle changed the title feat: Stop accepting setting layout as a graph attribute if it is a matrix feat!: Stop accepting setting layout as a graph attribute if it is a matrix Mar 18, 2024
@krlmlr
Copy link
Contributor Author

krlmlr commented Mar 19, 2024

@szhorvat: Something like 20dc1e4?

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.

Disturbed layout in subgraph
3 participants