-
Notifications
You must be signed in to change notification settings - Fork 5
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
Plot 2d coords #17
Plot 2d coords #17
Conversation
tests/wrappers_test.py
Outdated
def test_plot_2d_coord_limits(): | ||
p = pp.plot(dense_data_array(ndim=2, ragged=True)) | ||
assert np.array_equal(p._ax.get_xlim(), [-0.5, 88.5]) | ||
assert np.array_equal(p._ax.get_ylim(), [-0.5, 49.5]) | ||
p = pp.plot(dense_data_array(ndim=2, ragged=True, binedges=True)) | ||
assert np.array_equal(p._ax.get_xlim(), [0.0, 89.0]) | ||
assert np.array_equal(p._ax.get_ylim(), [0.0, 50.0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests like this don't follow what we have, e.g., learned from the "Software Engineering @ Google@ book. This is just checking some numbers, it is unclear what they mean (since we do not see the data setup), and probably the test will break if dense_data_array
changes somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. So is doing assert np.array_equal(p._ax.get_xlim(), [da.coords['xx'].min().value, da.coords['xx'].max().value])
better? It still involves the xx
dim, which may change in the dense data array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, I think I will just remove this test. It sounds like it's going to break all the time, and it in fact has nothing to do with 2d coords, the problem was there also with 1d coords. It's the bin edges which is important.
src/plopp/mesh.py
Outdated
}).transpose(self._data.dims).values | ||
out = { | ||
'xy'[axis - 1]: | ||
np.repeat(broadcasted_coord, 2, axis=axis)[clip], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimonHeybrock do you have a good idea on how we could do more of this with scipp instead of numpy? I think the axis
parameter is a bit bug-prone, would be nice to just use dimension labels...
src/plopp/tools.py
Outdated
along dimension `dim`. | ||
""" | ||
index = x.dims.index(dim) + 1 | ||
dummy_dim = '_' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to break if that matches a dim name, use uuid.uuid4()
(grep in scipp
to see how we do this in other places).
src/plopp/mesh.py
Outdated
# Broadcast 1d coord to 2d and repeat along 1d dim | ||
broadcasted_coord = repeat(broadcast(xy[dim_1d[0]], | ||
sizes={ | ||
**xy[dim_2d[0]].sizes, | ||
**xy[dim_1d[0]].sizes | ||
}).transpose(self._data.dims), | ||
dim=dim_1d[1], | ||
n=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the repeat
and broadcast
be swapped? Is this expensive otherwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm would not be very easy I think. I can add a TODO note?
src/plopp/mesh.py
Outdated
if dim_2d is None: | ||
return xy['x'].values, xy['y'].values, z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this avoiding the potentially costly stuff below for "normal" coords?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
Fixes #3