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? |
src/scippnexus/nxtransformations.py
Outdated
| 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? |
|
src/scippnexus/nxtransformations.py
Outdated
| 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?
There was a problem hiding this comment.
I've moved the try-except to only cover the part of the code where it's relevant.
This makes the code less nested and easier to read.
I don't think it would benefit from being split into smaller functions at this point.
Co-authored-by: Jan-Lukas Wynen <j-l.wynen@hotmail.de>
…o transform-visualize
All the values/attrs that are part of a transform. If you omit some it may be misinterpreted as a bug, and not seeing all of them means it is impossible to interpret what the transform does. There are not many, so I think including all should be perfectly feasible? |
SimonHeybrock
left a comment
SimonHeybrock
left a comment
There was a problem hiding this comment.
Thanks, code looks good. Can you share some screenshots of the latest state?
@jl-wynen Did you have more comments?
|
4 participants
Fixes #292