Conversation
|
|
I think it would be useful to show the values as well? How many meters it was translated by in what direction? Finally, I wonder if placing things at scale in the graph would be useful/possible for the translations? You could then build some sort of instrument layout with relative spacings between components? |
|
I added fields for the transformation vector and the magnitude. Placing things at scale I think wouldn't be so useful here. I interpreted this to be a more abstract illustration to debug/understand transformations in a nexus file. It would be difficult to make it look good, because some translations might be tiny and other might be large, and it would only cover the translations anyway (since we can't rotate out of the screen). |
|
Examples 
|
I think that would make it unusable in many cases. Let us not do that |
Can you also add add the others? No reason to omit anything, right? |
| f'{label}\\nvalue ∈ ' | ||
| f'[{_fmt(np.nanmin(vals))} {unit}, ' | ||
| f'{_fmt(np.nanmax(vals))} {unit}]' | ||
| ) |
There was a problem hiding this comment.
Why all the duplication and why not use Scipp?
src/scippnexus/nxtransformations.py
Outdated
| if path in visited: | ||
| raise ValueError( | ||
| f'Circular depends_on chain detected: {[*visited, path]}' | ||
| ) |
There was a problem hiding this comment.
Why not visualize this? The point of the visualization is to debug the chain. I don't see the need to raise here?
src/scippnexus/nxtransformations.py
Outdated
| except KeyError as e: | ||
| m = f'depends_on chain {depends_on} references missing node {e}'.replace( | ||
| '\n', '' | ||
| ) | ||
| warnings.warn(UserWarning(m), stacklevel=2) |
There was a problem hiding this comment.
Why is this not included in the graph? Also not sure why we would want to warn here? Can that just be a warning color on the graph?
Including too much information in the graphviz boxes will make the visualization hard to read and understand. What fields are you thinking about specifically? |
|
| dot.edge(prev, path, label='depends_on') | ||
| prev = path | ||
| depends_on = transform.depends_on | ||
| except KeyError: |
There was a problem hiding this comment.
This code is very long and deeply nested. And this try-except encompasses almost all of it. So it's difficult to understand how this works and what failures are supposed to trigger this fallback. Can you split this into smaller functions?
Co-authored-by: Jan-Lukas Wynen <j-l.wynen@hotmail.de>
4 participants
Fixes #292