REL: backport #960 to v2.5.2

CHANGELOG.md

@@ -5,6 +5,12 @@ The original MRs are only visible on the [LIGO GitLab repository](https://git.li
 
 ## [Unreleased]
 
+## [2.5.2]
+
+### Fixed
+
+- Fix the filename extension precedence for result files (https://github.com/bilby-dev/bilby/pull/960)
+
 ## [2.5.1]
 
 ### Changed
@@ -1151,7 +1157,8 @@ First `pip` installable version https://pypi.org/project/BILBY/ .
 - All chainconsumer dependency as this was causing issues.
 
 
-[Unreleased]: https://github.com/bilby-dev/bilby/compare/v2.5.1...main
+[Unreleased]: https://github.com/bilby-dev/bilby/compare/v2.5.2...main
+[2.5.2]: https://github.com/bilby-dev/bilby/compare/v2.5.1...v2.5.2
 [2.5.1]: https://github.com/bilby-dev/bilby/compare/v2.5.0...v2.5.1
 [2.5.0]: https://github.com/bilby-dev/bilby/compare/v2.4.0...v2.5.0
 [2.4.0]: https://github.com/bilby-dev/bilby/compare/v2.3.0...v2.4.0

bilby/core/result.py

@@ -768,9 +768,8 @@ def _get_save_data_dictionary(self):
         return dictionary
 
     def save_to_file(self, filename=None, overwrite=False, outdir=None,
-                     extension='json', gzip=False):
+                     extension=None, gzip=False):
         """
-
         Writes the Result to a file.
 
         Supported formats are: `json`, `hdf5`, `pickle`
@@ -785,22 +784,35 @@ def save_to_file(self, filename=None, overwrite=False, outdir=None,
         outdir: str, optional
             Path to the outdir. Default is the one stored in the result object.
             If given, overwrite path prefix in 'filename'.
-        extension: str, optional {json, hdf5, pkl, pickle, True}
-            Determines the method to use to store the data (if True defaults
-            to json)
+        extension: {"json", "hdf5", "pkl", None}, optional
+            Determines the method to use to store the data. If None, the extension
+            is inferred from the filename if provided, otherwise defaults to 'json'.
         gzip: bool, optional
             If true, and outputting to a json file, this will gzip the resulting
             file and add '.gz' to the file extension.
         """
 
-        if extension is True:
-            extension = "json"
-
         _outdir = None
         if filename is not None:
-            _outdir, filename = os.path.split(filename)
+            _outdir, base_filename = os.path.split(filename)
             _outdir = None if _outdir == "" else _outdir
-            filename = f"{os.path.splitext(filename)[0]}.{extension}"
+            base, ext = os.path.splitext(base_filename)
+            if extension is None:
+                extension = ext[1:] if ext else "json"
+                filename = base_filename
+            elif extension is True:
+                message = "Result.save_to_file called with extension=True. "
+                if len(ext) > 0:
+                    message += f"Overwriting extension to json from {ext}, this"
+                else:
+                    message += "This"
+                message += " behaviour is deprecated and will be removed."
+                logger.warning(message)
+                extension = "json"
+                filename = base_filename
+            filename = f"{base}.{extension}"
+        if extension is None or extension is True:
+            extension = 'json'
 
         outdir = _outdir if outdir is None else outdir
         outdir = self._safe_outdir_creation(outdir, self.save_to_file)
@@ -842,13 +854,13 @@ def save_to_file(self, filename=None, overwrite=False, outdir=None,
             elif extension == 'pkl':
                 safe_file_dump(self, output_path, "dill")
             else:
-                raise ValueError("Extension type {} not understood".format(extension))
+                raise ValueError(f"Extension type {extension} not understood")
         except Exception as e:
             output_path = f"{os.path.splitext(output_path)[0]}.pkl"
             safe_file_dump(self, output_path, "dill")
             logger.error(
                 "\n\nSaving the data has failed with the following message:\n"
-                "{}\nData has been dumped to {}.\n\n".format(e, output_path)
+                f"{e}\nData has been dumped to {output_path}.\n\n"
             )
 
     def save_posterior_samples(self, filename=None, outdir=None, label=None):

test/core/result_test.py

@@ -874,6 +874,49 @@ def test_reweight_different_likelihood_weights_correct(self):
         self.assertLess(abs(new.log_evidence - self.result.log_evidence), 0.05)
         self.assertNotEqual(new.log_evidence, self.result.log_evidence)
 
+    def test_save_to_file_filename_with_extension_and_extension_none(self):
+        # Should use the extension from filename
+        filename = os.path.join(self.result.outdir, "custom_name.hdf5")
+        self.result.save_to_file(filename=filename, extension=None)
+        self.assertTrue(os.path.isfile(filename))
+        os.remove(filename)
+
+    def test_save_to_file_filename_with_extension_and_extension_true(self):
+        """This is a strange default, that we should remove, but is here for consistency"""
+        filename = os.path.join(self.result.outdir, "custom_name.hdf5")
+        expected = os.path.join(self.result.outdir, "custom_name.json")
+        self.result.save_to_file(filename=filename, extension=True)
+        self.assertTrue(os.path.isfile(expected))
+        self.assertFalse(os.path.isfile(filename))
+        os.remove(expected)
+
+    def test_save_to_file_filename_with_extension_and_extension_set(self):
+        # Should override the extension in filename with the one provided in extension
+        filename = os.path.join(self.result.outdir, "custom_name.hdf5")
+        expected = os.path.join(self.result.outdir, "custom_name.json")
+        self.result.save_to_file(filename=filename, extension="json")
+        self.assertTrue(os.path.isfile(expected))
+        self.assertFalse(os.path.isfile(filename))
+        os.remove(expected)
+
+    def test_save_to_file_filename_without_extension_and_extension_none(self):
+        # Should use the default extension (json)
+        filename = os.path.join(self.result.outdir, "custom_name_noext")
+        expected = filename + ".json"
+        self.result.save_to_file(filename=filename, extension=None)
+        self.assertTrue(os.path.isfile(expected))
+        self.assertFalse(os.path.isfile(filename))
+        os.remove(expected)
+
+    def test_save_to_file_defaults_to_pickle_with_incorrect_extension(self):
+        """This is a weird fallback..."""
+        filename = os.path.join(self.result.outdir, "custom_name_noext")
+        expected = filename + ".pkl"
+        self.result.save_to_file(filename=filename, extension="bar")
+        self.assertTrue(os.path.isfile(expected))
+        self.assertFalse(os.path.isfile(filename))
+        os.remove(expected)
+
 
 if __name__ == "__main__":
     unittest.main()