From ba4aecb61592925d0865a32a15e6b366660fe9b3 Mon Sep 17 00:00:00 2001 From: prasad-sawantdesai Date: Mon, 17 Nov 2025 14:36:59 +0100 Subject: [PATCH 1/5] fix issues in coordinate validation --- imas/test/test_helpers.py | 125 ++++++++++++++++++++++++++++++-------- 1 file changed, 101 insertions(+), 24 deletions(-) diff --git a/imas/test/test_helpers.py b/imas/test/test_helpers.py index f398d03f..6bb365ef 100644 --- a/imas/test/test_helpers.py +++ b/imas/test/test_helpers.py @@ -117,27 +117,71 @@ def maybe_set_random_value( primitive.value = random_data(primitive.metadata.data_type, ndim) return + for dim, same_as in enumerate(primitive.metadata.coordinates_same_as): + if same_as.references: + try: + ref_elem = same_as.references[0].goto(primitive) + if len(ref_elem.shape) <= dim or ref_elem.shape[dim] == 0: + return + except (ValueError, AttributeError, IndexError, RuntimeError): + return + + if primitive.metadata.name.endswith("_error_upper"): + name = primitive.metadata.name[: -len("_error_upper")] + try: + data = primitive._parent[name] + except (KeyError, AttributeError): + return + if ( + not data.has_value + or len(data.shape) == 0 + or any(s == 0 for s in data.shape) + ): + return + if any( + same_as.references for same_as in primitive.metadata.coordinates_same_as + ): + return + elif primitive.metadata.name.endswith("_error_lower"): + name = primitive.metadata.name[: -len("_error_lower")] + "_error_upper" + try: + data = primitive._parent[name] + except (KeyError, AttributeError): + return + if ( + not data.has_value + or len(data.shape) == 0 + or any(s == 0 for s in data.shape) + ): + return + if any( + same_as.references for same_as in primitive.metadata.coordinates_same_as + ): + return + shape = [] for dim, coordinate in enumerate(primitive.metadata.coordinates): same_as = primitive.metadata.coordinates_same_as[dim] - if not coordinate.has_validation and not same_as.has_validation: - if primitive.metadata.name.endswith("_error_upper"): - # _error_upper should only be filled when is - name = primitive.metadata.name[: -len("_error_upper")] - data = primitive._parent[name] - if not data.has_value: - return - size = data.shape[dim] - elif primitive.metadata.name.endswith("_error_lower"): - # _error_lower should only be filled when _error_upper is - name = primitive.metadata.name[: -len("_error_lower")] + "_error_upper" - data = primitive._parent[name] - if not data.has_value: - return - size = data.shape[dim] - else: - # we can independently choose a size for this dimension: - size = random.randint(1, 6) + + if primitive.metadata.name.endswith("_error_upper"): + name = primitive.metadata.name[: -len("_error_upper")] + data = primitive._parent[name] + if dim >= len(data.shape): + return + size = data.shape[dim] + if size == 0: + return + elif primitive.metadata.name.endswith("_error_lower"): + name = primitive.metadata.name[: -len("_error_lower")] + "_error_upper" + data = primitive._parent[name] + if dim >= len(data.shape): + return + size = data.shape[dim] + if size == 0: + return + elif not coordinate.has_validation and not same_as.has_validation: + # we can independently choose a size for this dimension: + size = random.randint(1, 6) elif coordinate.references or same_as.references: try: if coordinate.references: @@ -147,8 +191,8 @@ def maybe_set_random_value( coordinate_element = filled_refs[0] if filled_refs else refs[0] else: coordinate_element = same_as.references[0].goto(primitive) - except (ValueError, AttributeError): - # Ignore invalid coordinate specs + except (ValueError, AttributeError, IndexError): + # Ignore invalid coordinate specs or empty array references coordinate_element = np.ones((1,) * 6) if len(coordinate_element) == 0: @@ -269,10 +313,43 @@ def fill_consistent( elif any(len(coordinate.references) > 1 for coordinate in coordinates): exclusive_coordinates.append(child) else: - try: - maybe_set_random_value(child, leave_empty, skip_complex) - except (RuntimeError, ValueError): - pass + same_as_skip = False + for dim, same_as in enumerate(child.metadata.coordinates_same_as): + if same_as.references: + try: + ref_elem = same_as.references[0].goto(child) + if len(ref_elem.shape) <= dim or ref_elem.shape[dim] == 0: + same_as_skip = True + break + except (ValueError, AttributeError, IndexError, RuntimeError): + same_as_skip = True + break + + error_skip = False + if child.metadata.name.endswith("_error_upper"): + name = child.metadata.name[: -len("_error_upper")] + data = child._parent[name] + if ( + not data.has_value + or len(data.shape) == 0 + or any(s == 0 for s in data.shape) + ): + error_skip = True + elif child.metadata.name.endswith("_error_lower"): + name = child.metadata.name[: -len("_error_lower")] + "_error_upper" + data = child._parent[name] + if ( + not data.has_value + or len(data.shape) == 0 + or any(s == 0 for s in data.shape) + ): + error_skip = True + + if not same_as_skip and not error_skip: + try: + maybe_set_random_value(child, leave_empty, skip_complex) + except (RuntimeError, ValueError): + pass if isinstance(structure, IDSToplevel): # handle exclusive_coordinates From 842f170be314ba79d86b11808d67eb0693d035f3 Mon Sep 17 00:00:00 2001 From: prasad-sawantdesai Date: Mon, 17 Nov 2025 15:48:38 +0100 Subject: [PATCH 2/5] first fill the data and check later --- imas/test/test_helpers.py | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/imas/test/test_helpers.py b/imas/test/test_helpers.py index 6bb365ef..e8bd6702 100644 --- a/imas/test/test_helpers.py +++ b/imas/test/test_helpers.py @@ -328,21 +328,23 @@ def fill_consistent( error_skip = False if child.metadata.name.endswith("_error_upper"): name = child.metadata.name[: -len("_error_upper")] - data = child._parent[name] - if ( - not data.has_value - or len(data.shape) == 0 - or any(s == 0 for s in data.shape) - ): + try: + data = child._parent[name] + if not data.has_value: + maybe_set_random_value(data, 0.0, skip_complex) + if not data.has_value or len(data.shape) == 0 or any(s == 0 for s in data.shape): + error_skip = True + except (KeyError, AttributeError, RuntimeError, ValueError): error_skip = True elif child.metadata.name.endswith("_error_lower"): name = child.metadata.name[: -len("_error_lower")] + "_error_upper" - data = child._parent[name] - if ( - not data.has_value - or len(data.shape) == 0 - or any(s == 0 for s in data.shape) - ): + try: + data = child._parent[name] + if not data.has_value: + maybe_set_random_value(data, 0.0, skip_complex) + if not data.has_value or len(data.shape) == 0 or any(s == 0 for s in data.shape): + error_skip = True + except (KeyError, AttributeError, RuntimeError, ValueError): error_skip = True if not same_as_skip and not error_skip: From 671833a01121af314829d68e80603f6640373b97 Mon Sep 17 00:00:00 2001 From: prasad-sawantdesai Date: Tue, 18 Nov 2025 09:22:26 +0100 Subject: [PATCH 3/5] fixed black formatting --- imas/test/test_helpers.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/imas/test/test_helpers.py b/imas/test/test_helpers.py index e8bd6702..ae8784aa 100644 --- a/imas/test/test_helpers.py +++ b/imas/test/test_helpers.py @@ -332,7 +332,11 @@ def fill_consistent( data = child._parent[name] if not data.has_value: maybe_set_random_value(data, 0.0, skip_complex) - if not data.has_value or len(data.shape) == 0 or any(s == 0 for s in data.shape): + if ( + not data.has_value + or len(data.shape) == 0 + or any(s == 0 for s in data.shape) + ): error_skip = True except (KeyError, AttributeError, RuntimeError, ValueError): error_skip = True @@ -342,7 +346,11 @@ def fill_consistent( data = child._parent[name] if not data.has_value: maybe_set_random_value(data, 0.0, skip_complex) - if not data.has_value or len(data.shape) == 0 or any(s == 0 for s in data.shape): + if ( + not data.has_value + or len(data.shape) == 0 + or any(s == 0 for s in data.shape) + ): error_skip = True except (KeyError, AttributeError, RuntimeError, ValueError): error_skip = True From 241b5a4af269cf330e6a14cc4efe5ef32679a9e4 Mon Sep 17 00:00:00 2001 From: prasad-sawantdesai Date: Thu, 20 Nov 2025 10:31:45 +0100 Subject: [PATCH 4/5] fix comments suggested by Maarten --- imas/test/test_helpers.py | 146 ++++++++++---------------------------- 1 file changed, 39 insertions(+), 107 deletions(-) diff --git a/imas/test/test_helpers.py b/imas/test/test_helpers.py index ae8784aa..a27c4ec9 100644 --- a/imas/test/test_helpers.py +++ b/imas/test/test_helpers.py @@ -126,12 +126,10 @@ def maybe_set_random_value( except (ValueError, AttributeError, IndexError, RuntimeError): return + shape = [] if primitive.metadata.name.endswith("_error_upper"): name = primitive.metadata.name[: -len("_error_upper")] - try: - data = primitive._parent[name] - except (KeyError, AttributeError): - return + data = primitive._parent[name] if ( not data.has_value or len(data.shape) == 0 @@ -142,12 +140,10 @@ def maybe_set_random_value( same_as.references for same_as in primitive.metadata.coordinates_same_as ): return + shape = list(data.shape) elif primitive.metadata.name.endswith("_error_lower"): name = primitive.metadata.name[: -len("_error_lower")] + "_error_upper" - try: - data = primitive._parent[name] - except (KeyError, AttributeError): - return + data = primitive._parent[name] if ( not data.has_value or len(data.shape) == 0 @@ -158,60 +154,39 @@ def maybe_set_random_value( same_as.references for same_as in primitive.metadata.coordinates_same_as ): return + shape = list(data.shape) + else: + for dim, coordinate in enumerate(primitive.metadata.coordinates): + same_as = primitive.metadata.coordinates_same_as[dim] - shape = [] - for dim, coordinate in enumerate(primitive.metadata.coordinates): - same_as = primitive.metadata.coordinates_same_as[dim] - - if primitive.metadata.name.endswith("_error_upper"): - name = primitive.metadata.name[: -len("_error_upper")] - data = primitive._parent[name] - if dim >= len(data.shape): - return - size = data.shape[dim] - if size == 0: - return - elif primitive.metadata.name.endswith("_error_lower"): - name = primitive.metadata.name[: -len("_error_lower")] + "_error_upper" - data = primitive._parent[name] - if dim >= len(data.shape): - return - size = data.shape[dim] + if not coordinate.has_validation and not same_as.has_validation: + # we can independently choose a size for this dimension: + size = random.randint(1, 6) + elif coordinate.references or same_as.references: + try: + if coordinate.references: + refs = [ref.goto(primitive) for ref in coordinate.references] + filled_refs = [ref for ref in refs if len(ref) > 0] + assert len(filled_refs) in (0, 1) + coordinate_element = filled_refs[0] if filled_refs else refs[0] + else: + coordinate_element = same_as.references[0].goto(primitive) + except (ValueError, AttributeError, IndexError): + # Ignore invalid coordinate specs or empty array references + coordinate_element = np.ones((1,) * 6) + + if len(coordinate_element) == 0: + maybe_set_random_value(coordinate_element, 0.5**ndim, skip_complex) + size = coordinate_element.shape[0 if coordinate.references else dim] + + if coordinate.size: # coordinateX = OR 1...1 + if random.random() < 0.5: + size = coordinate.size + else: + size = coordinate.size if size == 0: - return - elif not coordinate.has_validation and not same_as.has_validation: - # we can independently choose a size for this dimension: - size = random.randint(1, 6) - elif coordinate.references or same_as.references: - try: - if coordinate.references: - refs = [ref.goto(primitive) for ref in coordinate.references] - filled_refs = [ref for ref in refs if len(ref) > 0] - assert len(filled_refs) in (0, 1) - coordinate_element = filled_refs[0] if filled_refs else refs[0] - else: - coordinate_element = same_as.references[0].goto(primitive) - except (ValueError, AttributeError, IndexError): - # Ignore invalid coordinate specs or empty array references - coordinate_element = np.ones((1,) * 6) - - if len(coordinate_element) == 0: - # Scale chance of not setting a coordinate by our number of dimensions, - # such that overall there is roughly a 50% chance that any coordinate - # remains empty - maybe_set_random_value(coordinate_element, 0.5**ndim, skip_complex) - size = coordinate_element.shape[0 if coordinate.references else dim] - - if coordinate.size: # coordinateX = OR 1...1 - # Coin flip whether to use the size as determined by - # coordinate.references, or the size from coordinate.size - if random.random() < 0.5: - size = coordinate.size - else: - size = coordinate.size - if size == 0: - return # Leave empty - shape.append(size) + return # Leave empty + shape.append(size) if primitive.metadata.data_type is IDSDataType.STR: primitive.value = [random_string() for i in range(shape[0])] @@ -313,53 +288,10 @@ def fill_consistent( elif any(len(coordinate.references) > 1 for coordinate in coordinates): exclusive_coordinates.append(child) else: - same_as_skip = False - for dim, same_as in enumerate(child.metadata.coordinates_same_as): - if same_as.references: - try: - ref_elem = same_as.references[0].goto(child) - if len(ref_elem.shape) <= dim or ref_elem.shape[dim] == 0: - same_as_skip = True - break - except (ValueError, AttributeError, IndexError, RuntimeError): - same_as_skip = True - break - - error_skip = False - if child.metadata.name.endswith("_error_upper"): - name = child.metadata.name[: -len("_error_upper")] - try: - data = child._parent[name] - if not data.has_value: - maybe_set_random_value(data, 0.0, skip_complex) - if ( - not data.has_value - or len(data.shape) == 0 - or any(s == 0 for s in data.shape) - ): - error_skip = True - except (KeyError, AttributeError, RuntimeError, ValueError): - error_skip = True - elif child.metadata.name.endswith("_error_lower"): - name = child.metadata.name[: -len("_error_lower")] + "_error_upper" - try: - data = child._parent[name] - if not data.has_value: - maybe_set_random_value(data, 0.0, skip_complex) - if ( - not data.has_value - or len(data.shape) == 0 - or any(s == 0 for s in data.shape) - ): - error_skip = True - except (KeyError, AttributeError, RuntimeError, ValueError): - error_skip = True - - if not same_as_skip and not error_skip: - try: - maybe_set_random_value(child, leave_empty, skip_complex) - except (RuntimeError, ValueError): - pass + try: + maybe_set_random_value(child, leave_empty, skip_complex) + except (RuntimeError, ValueError): + pass if isinstance(structure, IDSToplevel): # handle exclusive_coordinates From 4f79b213a752705980a23df86753b11b56c977c3 Mon Sep 17 00:00:00 2001 From: Maarten Sebregts Date: Thu, 20 Nov 2025 15:33:57 +0100 Subject: [PATCH 5/5] Fix issue with fill_consistent and simplify logic - Errorbars were not cleared by `unset_coordinate()`, this is fixed now - Use imas.util.tree_iter in unset_coordinate, which is more clear than visit_children and performs slightly better - Simplify some checks in `maybe_set_random_value()` --- imas/test/test_helpers.py | 42 ++++++++++++++++----------------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/imas/test/test_helpers.py b/imas/test/test_helpers.py index a27c4ec9..cb6d107d 100644 --- a/imas/test/test_helpers.py +++ b/imas/test/test_helpers.py @@ -19,7 +19,7 @@ from imas.ids_struct_array import IDSStructArray from imas.ids_structure import IDSStructure from imas.ids_toplevel import IDSToplevel -from imas.util import idsdiffgen, visit_children +from imas.util import idsdiffgen, tree_iter logger = logging.getLogger(__name__) @@ -130,29 +130,13 @@ def maybe_set_random_value( if primitive.metadata.name.endswith("_error_upper"): name = primitive.metadata.name[: -len("_error_upper")] data = primitive._parent[name] - if ( - not data.has_value - or len(data.shape) == 0 - or any(s == 0 for s in data.shape) - ): - return - if any( - same_as.references for same_as in primitive.metadata.coordinates_same_as - ): + if not data.has_value: return shape = list(data.shape) elif primitive.metadata.name.endswith("_error_lower"): name = primitive.metadata.name[: -len("_error_lower")] + "_error_upper" data = primitive._parent[name] - if ( - not data.has_value - or len(data.shape) == 0 - or any(s == 0 for s in data.shape) - ): - return - if any( - same_as.references for same_as in primitive.metadata.coordinates_same_as - ): + if not data.has_value: return shape = list(data.shape) else: @@ -317,21 +301,29 @@ def fill_consistent( def unset_coordinate(coordinate): + def unset(element): + # Unset element value + element.value = [] + # But also its errorbars (if they exist) + try: + element._parent[element.metadata.name + "_error_upper"].value = [] + element._parent[element.metadata.name + "_error_lower"].value = [] + except AttributeError: + pass # Ignore when element has no errorbars + # Unset the coordinate quantity - coordinate.value = [] + unset(coordinate) # Find all elements that also have this as a coordinate and unset... parent = coordinate._dd_parent while parent.metadata.data_type is not IDSDataType.STRUCT_ARRAY: parent = parent._dd_parent - def callback(element): + for element in tree_iter(parent): if hasattr(element, "coordinates") and element.has_value: for ele_coor in element.coordinates: if ele_coor is coordinate: - element.value = [] - return - - visit_children(callback, parent) + unset(element) + break def compare_children(st1, st2, deleted_paths=set(), accept_lazy=False):