Repository: wright-group/WrightTools

Issue:
collection.convert
Would like to support syntax `collection.convert(unit)`.

Would convert all contained data objects recursively.

Discussion:
kameyer226: Weirdness with git I have to puzzle out, resolved by making changes to a new branch instead. 
untzag: @kameyer226 please try to keep a single change within a single branch. You can commit many times to the same branch and the same PR. Feel free to reach out or look online if you are having problems with git. Remember---read the messages carefully and git often tells you exactly what to do :smile: 

When you open up many branches / PRs all about the same issue, it becomes challenging to see exactly what you're working on as a maintainer.

This resource might help: https://guides.github.com/introduction/flow/
kameyer226: I did the normal route, but I got an error saying the remote contains work I did not have locally.  Thinking someone modified the remote, I re pulled.    It then asked me which branch to merge with (917_KAM1 or pint), which didn't make sense to me, as why should my remote origin need to be respec'd after something so mundane as a pull off the branch I was working on?   
kameyer226: PS C:\work\wright\python\WrightTools\WrightTools> git commit -am "WIP_917_KAM1a"
[917_KAM1 b1c8ba6] WIP_917_KAM1a
PS C:\work\wright\python\WrightTools\WrightTools> git push
To https://github.com/wright-group/WrightTools
 ! [rejected]        917_KAM1 -> 917_KAM1 (fetch first)
error: failed to push some refs to 'https://github.com/wright-group/WrightTools'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
PS C:\work\wright\python\WrightTools\WrightTools> git pull
remote: Enumerating objects: 54, done.
remote: Counting objects: 100% (54/54), done.
remote: Compressing objects: 100% (21/21), done.
remote: Total 54 (delta 38), reused 48 (delta 32), pack-reused 0
Unpacking objects: 100% (54/54), 10.90 KiB | 36.00 KiB/s, done.
From https://github.com/wright-group/WrightTools
   0a27d84..efdc3f1  917_KAM1   -> origin/917_KAM1
 + d870114...f43731f pint       -> origin/pint  (forced update)
Please specify which branch you want to merge with.
See git-pull(1) for details.

    git pull <remote> <branch>

If you wish to set tracking information for this branch you can do so with:

    git branch --set-upstream-to=origin/<branch> 917_KAM1

PS C:\work\wright\python\WrightTools\WrightTools>
untzag: Yep, just do what git is asking:

```
git branch --set-upstream-to=origin/917_KAM1 917_KAM1
```
ksunden: I would also suggest adding at least the `verbose` kwarg, maybe even `convert_variables` (each just passed into the underlying calls)

This will keep the api consistent, by providing the same options.

https://github.com/wright-group/WrightTools/blob/123a9b1c922e83c5e1c3b39c39279aea134e80ce/WrightTools/data/_data.py#L761
untzag: @kameyer226 could you please update the title and description of this PR? Thanks :smile: 
ksunden: @kameyer226 Can you please put a descriptive title and description in this PR (you can edit with the button on the top right)

That will help others to know what is going on without needing to go searching for the issue.

You should put something like "Closes #917" in the description, this will link the PR to the issue so that the issue will be automatically closed upon merge of the PR.

Additionally, I highly recommend having descriptive commit messages:

e.g.:
`git commit -am "remove infinite recursion"`

(note that the quotes allow you to have spaces, even new lines)
ksunden: Note that it only gets linked properly if the `Closes #917` is in the _description_ (not the title... its fine for it to be there as well, just doesn't do the auto close)
kameyer226: Perhaps I am not certain what units are to be converted.   Calling `Collection()` currently does not instantiate a `Data` object, however, by calling `Collection.create_data()` a Data object becomes instantiated.   I thought one only needed to convert the units of that Data object.
ksunden: A collection is is a folder of potentially many data objects.

when you do `self.data` you are only getting one data object, and only then if it is itself called "data" (the default for `create_data`, but by no means guaranteed to exist)

This method should loop over _all_ data objects (and indeed nested collections, which are also allowed), no matter their names.
ksunden: http://wright.tools/en/stable/collection.html
kameyer226: I see, I had not come across an example where a collection was nested inside a collection.    Does this nesting continue beyond one level?
ksunden: It can, but that does not matter in the slightest to the implementation.
ksunden: You do not need to do _any_ isinstance check
kameyer226: Results of script run:   (Forgot to ask how valid test scripts should be reported to reviewers)

axis w1 converted from wn to eV
axis w2 converted from wn to eV
axis w1 converted from wn to eV
axis w2 converted from wn to eV
axis w1 converted from wn to eV
axis w2 converted from wn to eV
axis w1 converted from wn to eV
axis w2 converted from wn to eV
axis w1 converted from wn to eV
axis w2 converted from wn to eV
axis w1 converted from wn to eV
axis w2 converted from wn to eV
axis w1 converted from wn to eV
axis w2 converted from wn to eV
axis w1 converted from wn to eV
axis w2 converted from wn to eV
True
True
True
True
True
True
True
True
kameyer226: Sorry to respond late to @untzag :  I left the isinstances separate because I wanted to create the impression that the collection may consist of objects that are not type Data
kameyer226: Somehow .vscode is not in the exceptions to git, guess I better delete...

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9334bf8..9a7bbc7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ## [3.3.3]
 
+## Added
+- Collection.convert method for converting all units of a single kind of a collection's data to another unit (with test script)
+
 ### Fixed
 - Further handling for readonly files
 - Improved chopping with axes that span the kept axes removed
diff --git a/WrightTools/collection/_collection.py b/WrightTools/collection/_collection.py
index 7ca9048..80b3f8f 100644
--- a/WrightTools/collection/_collection.py
+++ b/WrightTools/collection/_collection.py
@@ -102,6 +102,18 @@ class Collection(Group):
         setattr(self, name, collection)
         return collection
 
+    def convert(self, units, convert_variables=False, verbose=True):
+        """Convert units of a collection.
+        Parameters
+        ----------
+        units: string
+            Units to convert to.
+        """
+
+        for name in self.item_names:
+            item = self[name]
+            item.convert(units, convert_variables=convert_variables, verbose=verbose)
+
     def create_data(self, name="data", position=None, **kwargs):
         """Create a new child data.
 


Test patch:
diff --git a/tests/collection/convert.py b/tests/collection/convert.py
new file mode 100644
index 0000000..2cf255c
--- /dev/null
+++ b/tests/collection/convert.py
@@ -0,0 +1,83 @@
+import WrightTools as wt
+import numpy as np
+
+
+def test_collection_convert():
+    def my_resonance(xi, yi, intensity=1, FWHM=500, x0=7000):
+        def single(arr, intensity=intensity, FWHM=FWHM, x0=x0):
+            return intensity * (0.5 * FWHM) ** 2 / ((xi - x0) ** 2 + (0.5 * FWHM) ** 2)
+
+        return single(xi) * single(yi)
+
+    xi = np.linspace(6000, 8000, 75)[:, None]
+    yi = np.linspace(6000, 8000, 75)[None, :]
+    zi = my_resonance(xi, yi)
+
+    results = wt.Collection(name="results")
+    results.create_data(name="neat")
+    results.neat.create_variable(name="T1", units="fs", values=xi)
+    results.neat.create_variable(name="w2", units="wn", values=yi)
+    results.neat.create_channel(name="signal", values=zi)
+    results.neat.transform("T1", "w2")
+
+    results.create_data(name="messy")
+    results.messy.create_variable(name="w1", units="wn", values=xi)
+    results.messy.create_variable(name="w2", units="wn", values=yi)
+    results.messy.create_channel(name="signal", values=zi)
+    results.messy.transform("w1", "w2")
+
+    results.create_data(name="confusing")
+    results.confusing.create_variable(name="w1", units="wn", values=xi)
+    results.confusing.create_variable(name="w2", units="wn", values=yi)
+    results.confusing.create_channel(name="signal", values=zi)
+    results.confusing.transform("w1", "w2")
+
+    calibration = results.create_collection(name="calibration")
+    calibration.create_data(name="OPA1_tune_test")
+    calibration.OPA1_tune_test.create_variable(name="w1", units="wn", values=xi)
+    calibration.OPA1_tune_test.create_variable(name="w2", units="wn", values=yi)
+    calibration.OPA1_tune_test.create_channel(name="signal", values=zi)
+    calibration.OPA1_tune_test.transform("w1", "w2")
+
+    calibration.create_data(name="OPA2_tune_test")
+    calibration.OPA2_tune_test.create_variable(name="w1", units="wn", values=xi)
+    calibration.OPA2_tune_test.create_variable(name="w2", units="wn", values=yi)
+    calibration.OPA2_tune_test.create_channel(name="signal", values=zi)
+    calibration.OPA2_tune_test.transform("w1", "w2")
+
+    displays = results.create_collection(name="displays")
+    displays.create_data(name="figure1")
+    displays.figure1.create_variable(name="w1", units="wn", values=xi)
+    displays.figure1.create_variable(name="w2", units="wn", values=yi)
+    displays.figure1.create_channel(name="signal", values=zi)
+    displays.figure1.transform("w1", "w2")
+
+    displayfig2 = displays.create_collection(name="figure2")
+    displayfig2.create_data(name="main")
+    displayfig2.main.create_variable(name="w1", units="wn", values=xi)
+    displayfig2.main.create_variable(name="w2", units="wn", values=yi)
+    displayfig2.main.create_channel(name="signal", values=zi)
+    displayfig2.main.transform("w1", "w2")
+
+    displayfig2.create_data(name="inset")
+    displayfig2.inset.create_variable(name="w1", units="wn", values=xi)
+    displayfig2.inset.create_variable(name="w2", units="wn", values=yi)
+    displayfig2.inset.create_channel(name="signal", values=zi)
+    displayfig2.inset.transform("w1", "w2")
+
+    results.convert("eV")
+
+    # alternating indices should be enough to prove all are eV
+    assert results.neat.units[0] == "fs"  # verifies conversion of diff. kind is excluded
+    assert results.neat.units[1] == "eV"
+    assert results.messy.units[1] == "eV"
+    assert results.confusing.units[0] == "eV"
+    assert results.calibration.OPA1_tune_test.units[1] == "eV"
+    assert results.calibration.OPA2_tune_test.units[0] == "eV"
+    assert results.displays.figure1.units[1] == "eV"
+    assert results.displays.figure2.main.units[0] == "eV"
+    assert results.displays.figure2.inset.units[1] == "eV"
+
+
+if __name__ == "__main__":
+    test_collection_convert()