Improve test_create_decode_mask

[zweihuehner]

I feel like the test test_create_decode_mask should be more specific. Currently, it verifies that the total number of edges decreases when applying a decode mask. However, checking the m2g (decoding) edges alone should be sufficient.

I don't think the test can fail in the current version. However, I encountered issues on the research branch where the decode_mask also affects the g2m behavior, which can increase the total number of edges in the graph. This made me realize that it would be more future-proof to make the test more specific.

weather-model-graphs/tests/test_graph_creation.py

Lines 209 to 218 in ea2415f

	unfiltered_graph = fn(coords=xy, mesh_node_distance=mesh_node_distance)
	# Filter to only 20 / 100 grid nodes
	decode_mask = np.concatenate((np.ones(20), np.zeros(80))).astype(bool)
	filtered_graph = fn(
	coords=xy, mesh_node_distance=mesh_node_distance, decode_mask=decode_mask
	)
	# Check that some filtering has been performed
	assert len(filtered_graph.edges) < len(unfiltered_graph.edges)

[bhawna346]

Hi, I would like to work on this issue. Could you please assign it to me?

[leifdenby]

I feel like the test test_create_decode_mask should be more specific. Currently, it verifies that the total number of edges decreases when applying a decode mask. However, checking the m2g (decoding) edges alone should be sufficient.

Could we argue that the test is currently buggy since it doesn't narrowly check exactly the aspect of the graph we trying to check? In that case we could bring a "fix" here as-is, otherwise we should consider this a "feature"

[zweihuehner]

Hi, I would like to work on this issue. Could you please assign it to me?

Good energy! ;) However, I’ve already opened a PR here in case this is deemed relevant, but please have a look if you agree this is actually the safer testing approach: #104

---

I feel like the test test_create_decode_mask should be more specific. Currently, it verifies that the total number of edges decreases when applying a decode mask. However, checking the m2g (decoding) edges alone should be sufficient.

Could we argue that the test is currently buggy since it doesn't narrowly check exactly the aspect of the graph we trying to check? In that case we could bring a "fix" here as-is, otherwise we should consider this a "feature"

I wouldn’t necessarily call the current test a bug, but it is somewhat vague. From the name, I would expect the test to focus specifically on the decoding part of the graph. At the moment, it checks whether the total number of edges changes, which implicitly assumes that decode_mask only affects m2g.

This works fine with the current implementation. However, it could become problematic if decode_mask is extended to influence other parts of the graph as well—something that already seems to happen in the research_branch version. While that implementation might not be final, it highlights a potential future issue.

To make the test more robust and aligned with its intent, it might be better to explicitly validate only the m2g edges. That way, we avoid unintended side effects and keep the test focused.

Of course, happy to go with the current approach if you prefer—just wanted to flag this for future consideration 🙂

[bhawna346]

Thanks for the detailed feedback! That makes sense — the current test is indeed too broad and doesn't strictly validate the decoding (m2g) edges.

I’ll update the test to specifically check the m2g edges to make it more precise and aligned with the intended behavior. I’ll push an updated version shortly.

[bhawna346]

Thanks for the helpful feedback! I've updated the test to explicitly validate that only edges where both nodes satisfy the decode mask are retained.

Additionally, I added assertions to ensure the test strictly verifies the intended behavior of m2g edge filtering. Please let me know if this aligns better with your expectations.

[zweihuehner]

Thanks for the effort! Since a PR already exists, I’ve marked yours as a duplicate of #104.
I was also so free to close it, to avoid cluttering.

That said, I did take some inspiration from your title—it was more specific than #104😉