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

Fix value for circular streams #44

Closed
SimonSimCity opened this issue Aug 29, 2023 · 4 comments
Closed

Fix value for circular streams #44

SimonSimCity opened this issue Aug 29, 2023 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@SimonSimCity
Copy link

SimonSimCity commented Aug 29, 2023

Thanks for providing this sankey diagram visualization! I'm actually quite happy it doesn't collapse when adding a circular stream. Just this issue: The value doesn't seem to be right...

Example: https://sankeydiagram.net/?content=PTAEGUHkFUCUGECioDaA1AggGWsgFAMYCGAdqAEYCmoRoA5APx0CUAuqACoawDiiHoPCniQskWK2YAoAO5EA5tRQN2ASxIA3SgGcALuvlT1WvQdQrQ/AGLap17agCMAVgAM7cADIACm9d2OGyc3dkQAVwAnAHtwXSiAD3i3I00dfRJ5c3Y9KIIAa1sc/IcUENBjNIMpIA===&

sankeydiagram-net-export

// SOURCE [VALUE (can be a '?')] TARGET ([COLOR])
wage [?] investing
investing [?] ETFs
ETFs [150] S&P500
ETFs [150] EuroStoxx50
investing [?] stocks
stocks [50] investing

In the current implementation, the node investing has a value of 400, whereby as I read it, it should only be 300, because the 50 are straight fed back into the node, increasing it by another 50.

I've found another library which contains a reference to an algorithm to detect circular references - maybe it helps: https://github.com/tomshanley/d3-sankey-circular

@SimonSimCity SimonSimCity changed the title Fix calculated value for circular streams Fix value for circular streams Aug 29, 2023
@JonasDoesThings
Copy link
Member

Hmm I've looked into this issue and indeed 400 is completely wrong.

That said, I would lean to the side of 350 being to correct value of "investing". It's true that the -50 and +50 are a zero-sum game, but it's still 300 and 50 units going into the "investing" node (see my graphic).

image

What's your opinion on that @SimonSimCity?

Nonetheless I will release an update to the dev branch shortly that fixes the incorrect behavior causing 400 to be the node's calculated value. After making sure that it doesn't break any legitimate normal use, I will release it to the main branch too.

@JonasDoesThings JonasDoesThings added the bug Something isn't working label Aug 29, 2023
@JonasDoesThings JonasDoesThings self-assigned this Aug 29, 2023
@JonasDoesThings
Copy link
Member

Update: I have deployed the preliminar fix to dev, you can try it out here

@SimonSimCity
Copy link
Author

You're right, 350 makes more sense 😎👍 Nice you've had time to fix it so quickly 🎉

@JonasDoesThings
Copy link
Member

The fix is live on https://sankeydiagram.net/ now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants