From 0cd2bd1aeffcee62fe2d1528b120eba52e42ec01 Mon Sep 17 00:00:00 2001 From: davidmcdonagh Date: Fri, 2 Feb 2024 14:09:18 +0000 Subject: [PATCH 1/8] xFixed bug in Scan.from_dict where some properties were not correctly parsed. --- newsfragments/XXX.misc | 1 + src/dxtbx/model/scan.py | 71 ++++++++++++++++++++++------------------ tests/model/test_scan.py | 6 ++++ 3 files changed, 46 insertions(+), 32 deletions(-) create mode 100644 newsfragments/XXX.misc diff --git a/newsfragments/XXX.misc b/newsfragments/XXX.misc new file mode 100644 index 000000000..96cdd168b --- /dev/null +++ b/newsfragments/XXX.misc @@ -0,0 +1 @@ +Fixed `Scan.from_dict` bug where some properties were not correctly parsed. diff --git a/src/dxtbx/model/scan.py b/src/dxtbx/model/scan.py index 40cf591c5..1789416a9 100644 --- a/src/dxtbx/model/scan.py +++ b/src/dxtbx/model/scan.py @@ -113,32 +113,42 @@ def from_dict(d, t=None): The scan model """ - def convert_oscillation_to_vec2(properties_dict): + def add_properties_table(scan_dict): """ - If oscillation is in properties_dict, - shared is converted to vec2 and - oscillation_width is removed (if present) to ensure - it is replaced correctly if updating t dict from d dict + Handles legacy case before Scan had a properties table. + Moves oscillation, epochs, and exposure times to a properties + table and adds this to scan_dict. """ - if "oscillation" not in properties_dict: - assert "oscillation_width" not in properties_dict - return properties_dict - if "oscillation_width" in properties_dict: - assert "oscillation" in properties_dict - properties_dict["oscillation"] = ( - properties_dict["oscillation"][0], - properties_dict["oscillation_width"][0], - ) - del properties_dict["oscillation_width"] - return properties_dict - properties_dict["oscillation"] = ( - properties_dict["oscillation"][0], - properties_dict["oscillation"][1] - properties_dict["oscillation"][0], - ) - - return properties_dict + properties = {} + if scan_dict: + image_range = scan_dict["image_range"] + num_images = 1 + image_range[1] - image_range[0] + if "oscillation" in scan_dict: + if num_images == 1: + properties["oscillation_width"] = [scan_dict["oscillation"][1]] + properties["oscillation"] = [scan_dict["oscillation"][0]] + + else: + osc = scan_dict["oscillation"] + properties["oscillation"] = [ + osc[0] + osc[1] * i for i in range(num_images) + ] + del scan_dict["oscillation"] + if "exposure_time" in scan_dict: + properties["exposure_time"] = scan_dict["exposure_time"] + del scan_dict["exposure_time"] + if "epochs" in scan_dict: + if len(scan_dict["epochs"]) != num_images: + properties["epochs"] = [ + scan_dict["epochs"][0] for i in scan_dict["epochs"] + ] + else: + properties["epochs"] = scan_dict["epochs"] + del scan_dict["epochs"] + scan_dict["properties"] = properties + return scan_dict if d is None and t is None: return None @@ -152,20 +162,17 @@ def convert_oscillation_to_vec2(properties_dict): joint.update(d) joint["properties"] = properties elif "properties" in d: + joint = add_properties_table(joint) d_copy = d.copy() - d_copy["properties"] = convert_oscillation_to_vec2( - d_copy["properties"].copy() - ) - joint.update(**d_copy["properties"]) + joint["properties"].update(d_copy["properties"]) del d_copy["properties"] joint.update(d_copy) elif "properties" in joint: - joint["properties"] = convert_oscillation_to_vec2( - joint["properties"].copy() - ) - joint.update(**joint["properties"]) - del joint["properties"] - joint.update(d) + d = add_properties_table(d) + d_copy = d.copy() + joint["properties"].update(d_copy["properties"]) + del d_copy["properties"] + joint.update(d_copy) else: joint.update(d) diff --git a/tests/model/test_scan.py b/tests/model/test_scan.py index 76dc3154a..4aceb62c3 100644 --- a/tests/model/test_scan.py +++ b/tests/model/test_scan.py @@ -471,3 +471,9 @@ def test_scan_is_still(): (1, 10), properties={"other_property": list(range(10))} ) assert scan.is_still() + +def test_scan_properties_from_dict(): + image_range = (1, 10) + properties = {"test": list(range(10))} + scan = ScanFactory.make_scan_from_properties(image_range, properties) + assert scan == ScanFactory.from_dict(scan.to_dict()) From 34053227c08ef296a865bd23d552f3508232567b Mon Sep 17 00:00:00 2001 From: davidmcdonagh Date: Fri, 2 Feb 2024 14:11:13 +0000 Subject: [PATCH 2/8] change newsfragment type --- newsfragments/{XXX.misc => XXX.bugfix} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename newsfragments/{XXX.misc => XXX.bugfix} (100%) diff --git a/newsfragments/XXX.misc b/newsfragments/XXX.bugfix similarity index 100% rename from newsfragments/XXX.misc rename to newsfragments/XXX.bugfix From e087f2cfd9af82803b1ce35610c86d70c0d94a7a Mon Sep 17 00:00:00 2001 From: DiamondLightSource-build-server Date: Fri, 2 Feb 2024 14:14:13 +0000 Subject: [PATCH 3/8] Rename newsfragments/XXX.bugfix to newsfragments/688.bugfix --- newsfragments/{XXX.bugfix => 688.bugfix} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename newsfragments/{XXX.bugfix => 688.bugfix} (100%) diff --git a/newsfragments/XXX.bugfix b/newsfragments/688.bugfix similarity index 100% rename from newsfragments/XXX.bugfix rename to newsfragments/688.bugfix From 068acffc2748c6fa21f484ae3553a053d0cd62d8 Mon Sep 17 00:00:00 2001 From: davidmcdonagh Date: Sat, 3 Feb 2024 21:57:15 +0000 Subject: [PATCH 4/8] Fixed further edge cases. --- src/dxtbx/model/scan.py | 88 +++++++++++++++++++++++++++++++++++------ 1 file changed, 75 insertions(+), 13 deletions(-) diff --git a/src/dxtbx/model/scan.py b/src/dxtbx/model/scan.py index 1789416a9..fe6c4f527 100644 --- a/src/dxtbx/model/scan.py +++ b/src/dxtbx/model/scan.py @@ -113,7 +113,7 @@ def from_dict(d, t=None): The scan model """ - def add_properties_table(scan_dict): + def add_properties_table(scan_dict, num_images): """ Handles legacy case before Scan had a properties table. @@ -123,8 +123,6 @@ def add_properties_table(scan_dict): properties = {} if scan_dict: - image_range = scan_dict["image_range"] - num_images = 1 + image_range[1] - image_range[0] if "oscillation" in scan_dict: if num_images == 1: properties["oscillation_width"] = [scan_dict["oscillation"][1]] @@ -133,44 +131,108 @@ def add_properties_table(scan_dict): else: osc = scan_dict["oscillation"] properties["oscillation"] = [ - osc[0] + osc[1] * i for i in range(num_images) + osc[0] + (osc[1] - osc[0]) * i for i in range(num_images) ] del scan_dict["oscillation"] if "exposure_time" in scan_dict: properties["exposure_time"] = scan_dict["exposure_time"] del scan_dict["exposure_time"] if "epochs" in scan_dict: - if len(scan_dict["epochs"]) != num_images: - properties["epochs"] = [ - scan_dict["epochs"][0] for i in scan_dict["epochs"] - ] - else: - properties["epochs"] = scan_dict["epochs"] + properties["epochs"] = scan_dict["epochs"] del scan_dict["epochs"] - scan_dict["properties"] = properties + scan_dict["properties"] = make_properties_table_consistent( + properties, num_images + ) return scan_dict + def make_properties_table_consistent(properties, num_images): + + """ + Handles legacy case before Scan had a properties table. + Ensures oscillation, epochs, and exposure times have the same length. + """ + + if not properties: + return properties + + if "oscillation" in properties: + assert len(properties["oscillation"]) > 0 + + if num_images == 1 and "oscillation_width" not in properties: + assert len(properties["oscillation"]) > 1 + properties["oscillation_width"] = [properties["oscillation"][1]] + properties["oscillation"] = [properties["oscillation"][0]] + elif num_images > 1: + osc_0 = properties["oscillation"][0] + if "oscillation_width" in properties: + osc_1 = properties["oscillation_width"][0] + del properties["oscillation_width"] + else: + assert len(properties["oscillation"]) > 1 + osc_1 = ( + properties["oscillation"][1] - properties["oscillation"][0] + ) + properties["oscillation"] = [ + osc_0 + osc_1 * i for i in range(num_images) + ] + + if "exposure_time" in properties: + assert len(properties["exposure_time"]) > 0 + + # Assume same exposure time for each image + properties["exposure_time"] = [ + properties["exposure_time"][0] for i in range(num_images) + ] + + if "epochs" in properties: + assert len(properties["epochs"]) > 0 + # If 1 epoch, assume increasing by epochs[0] + # Else assume increasing as epochs[1] - epochs[0] + if len(properties["epochs"]) == 1: + properties["epochs"] = [ + properties["epochs"][0] + properties["epochs"][0] * i + for i in range(num_images) + ] + else: + diff = properties["epochs"][1] - properties["epochs"][0] + properties["epochs"] = [ + properties["epochs"][0] + i * diff for i in range(num_images) + ] + return properties + if d is None and t is None: return None joint = t.copy() if t else {} # Accounting for legacy cases where t or d does not # contain properties dict + num_images = None + if "image_range" in d: + num_images = 1 + d["image_range"][1] - d["image_range"][0] + elif "image_range" in joint: + num_images = 1 + joint["image_range"][1] - joint["image_range"][0] + if "properties" in joint and "properties" in d: properties = t["properties"].copy() properties.update(d["properties"]) joint.update(d) joint["properties"] = properties elif "properties" in d: - joint = add_properties_table(joint) + joint = add_properties_table(joint, num_images) d_copy = d.copy() joint["properties"].update(d_copy["properties"]) + joint["properties"] = make_properties_table_consistent( + joint["properties"], num_images + ) del d_copy["properties"] joint.update(d_copy) elif "properties" in joint: - d = add_properties_table(d) + d = add_properties_table(d, num_images) d_copy = d.copy() joint["properties"].update(d_copy["properties"]) + joint["properties"] = make_properties_table_consistent( + joint["properties"], num_images + ) del d_copy["properties"] joint.update(d_copy) else: From 4495e6359540ee9001703c913c5b55128eef3ab9 Mon Sep 17 00:00:00 2001 From: davidmcdonagh Date: Sat, 10 Feb 2024 22:40:51 +0000 Subject: [PATCH 5/8] Added fix where oscillation_width was not converted to degrees in .expt. --- src/dxtbx/model/boost_python/scan.cc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/dxtbx/model/boost_python/scan.cc b/src/dxtbx/model/boost_python/scan.cc index 8a442d9f4..3c014e48f 100644 --- a/src/dxtbx/model/boost_python/scan.cc +++ b/src/dxtbx/model/boost_python/scan.cc @@ -76,7 +76,7 @@ namespace dxtbx { namespace model { namespace boost_python { value[0].attr("__class__").attr("__name__")); // Handled explicitly as it is in deg when serialised but rad in code - if (key == "oscillation") { + if (key == "oscillation" || key == "oscillation_width") { DXTBX_ASSERT(obj_type == "float"); scitbx::af::shared osc = boost::python::extract >(value); @@ -215,7 +215,14 @@ namespace dxtbx { namespace model { namespace boost_python { dxtbx::af::flex_table_suite::column_to_object_visitor visitor; for (const_iterator it = properties.begin(); it != properties.end(); ++it) { - if (it->first == "oscillation") { // Handled explicitly due to unit conversion + if (it->first + == "oscillation_width") { // Handled explicitly due to unit conversion + vec2 osc_deg = obj.get_oscillation_in_deg(); + boost::python::list lst = boost::python::list(); + lst.append(osc_deg[1]); + properties_dict[it->first] = lst; + } else if (it->first + == "oscillation") { // Handled explicitly due to unit conversion properties_dict[it->first] = boost::python::tuple(obj.get_oscillation_arr_in_deg()); } else { From 59f8f959be64122f2cf7b8896a6757d1ae500830 Mon Sep 17 00:00:00 2001 From: davidmcdonagh Date: Sat, 10 Feb 2024 23:07:22 +0000 Subject: [PATCH 6/8] additional checks in test_scan_properties_from_dict --- tests/model/test_scan.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/model/test_scan.py b/tests/model/test_scan.py index 4aceb62c3..3a17c4e59 100644 --- a/tests/model/test_scan.py +++ b/tests/model/test_scan.py @@ -477,3 +477,18 @@ def test_scan_properties_from_dict(): properties = {"test": list(range(10))} scan = ScanFactory.make_scan_from_properties(image_range, properties) assert scan == ScanFactory.from_dict(scan.to_dict()) + + image_range = (1, 1) + properties = {"oscillation": [1.0], "oscillation_width": [0.5]} + scan = ScanFactory.make_scan_from_properties(image_range, properties) + assert scan == ScanFactory.from_dict(scan.to_dict()) + + scan = ScanFactory.from_dict( + { + "oscillation": [1.0, 0.5], + "image_range": [1, 1], + "exposure_time": [0.5], + "epochs": [1], + } + ) + assert scan == ScanFactory.from_dict(scan.to_dict()) From 5beec3151be21699ce98cc85c8cf6c852c2c94fb Mon Sep 17 00:00:00 2001 From: davidmcdonagh Date: Mon, 12 Feb 2024 14:37:18 +0000 Subject: [PATCH 7/8] Added additional check to not modify Scan properties in Scan.from_dict if all properties already have the expected length. --- src/dxtbx/model/scan.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/dxtbx/model/scan.py b/src/dxtbx/model/scan.py index fe6c4f527..e63f2b025 100644 --- a/src/dxtbx/model/scan.py +++ b/src/dxtbx/model/scan.py @@ -155,6 +155,13 @@ def make_properties_table_consistent(properties, num_images): if not properties: return properties + property_length = len(next(iter(properties.values()))) + all_same_length = all( + len(lst) == property_length for lst in properties.values() + ) + if all_same_length and property_length == num_images: + return properties + if "oscillation" in properties: assert len(properties["oscillation"]) > 0 From ceaae2fb182323374a89ee34a4fe8e37a845b124 Mon Sep 17 00:00:00 2001 From: Ben Williams Date: Tue, 20 Feb 2024 10:54:26 +0000 Subject: [PATCH 8/8] Kick the CI