Repository: lbl-anp/becquerel

Issue:
Latest Compendium materials JSON causes errors
The latest version of the materials data from the [PNNL Compendium](https://compendium.cwmd.pnnl.gov) cannot be parsed by the current version of `becquerel`.

Downloading the latest JSON and placing it into `becquerel/tools` and calling `bq.tools.force_load_and_write_materials_csv()` causes the following error:

```
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File ".../becquerel/tools/materials.py", line 196, in force_load_and_write_materials_csv
    materials = _load_and_compile_materials()
  File ".../becquerel/tools/materials.py", line 26, in _load_and_compile_materials
    data_comp = fetch_compendium_data()
  File ".../becquerel/tools/materials_compendium.py", line 63, in fetch_compendium_data
    names = [datum["Name"] for datum in data]
  File ".../becquerel/tools/materials_compendium.py", line 63, in <listcomp>
    names = [datum["Name"] for datum in data]
TypeError: string indices must be integers
```

It seems from looking at a JSON from a few months ago and a JSON downloaded today that the difference is that the older JSON has a structure that parses into a `list` of `dict`s:

```json
[
    {
        "Comment": "Material 1 data",
    },
    {
        "Comment": "Material 2 data",
    }
]
```

whereas the new format parses as a `dict` and the list of material data are inside of it:

```json
{
    "siteVersion": "0.1.1",
    "data": [
        {
            "Comment": "Material 1 data",
        },
        {
            "Comment": "Material 2 data",
        }
    ]
}
```

So the solution is easy: to check for the data that get loaded and use `data["data"]` if it is the new format.

Both versions appear to contain 411 materials, but I have not checked whether any of the data for individual materials differ.

Discussion:
markbandstra: This PR is failing due to an issue I saw locally. I figured it was an error in `pre-commit`, `black`, or `click` but could not fix it and bypassed it. Here is the relevant error:

```
black....................................................................Failed
- hook id: black
- exit code: 1

Traceback (most recent call last):
  File "/home/runner/.cache/pre-commit/repoxi42o_5z/py_env-python3.9/bin/black", line 8, in <module>
    sys.exit(patched_main())
  File "/home/runner/.cache/pre-commit/repoxi42o_5z/py_env-python3.9/lib/python3.9/site-packages/black/__init__.py", line 1423, in patched_main
    patch_click()
  File "/home/runner/.cache/pre-commit/repoxi42o_5z/py_env-python3.9/lib/python3.9/site-packages/black/__init__.py", line 1409, in patch_click
    from click import _unicodefun
ImportError: cannot import name '_unicodefun' from 'click' (/home/runner/.cache/pre-commit/repoxi42o_5z/py_env-python3.9/lib/python3.9/site-packages/click/__init__.py)
```
jvavrek: I don't see the error on my machine:
- Python 3.8.10
- black 22.1.0
- click 8.0.1
jccurtis: @jvavrek yep I don't either BUT `click` was just updated a few hours ago: https://pypi.org/project/click/8.1.0/
markbandstra: Good point, here is my machine:

* Python 3.9.12
* pre-commit 2.17.0
* black 22.1.0
* click 8.0.4
 
jccurtis: Confirmed on a fresh install:

```bash
# Inside bq repo
conda create -n bq-test python=3.9
conda activate bq-test
pip install -r requirements-dev.txt
pip install -e .
pre-commit install
pre-commit run --all
```

gives:

```txt
❯ pre-commit run --all
Trim Trailing Whitespace.................................................Passed
Fix End of Files.........................................................Passed
Check for added large files..............................................Passed
Check JSON...........................................(no files to check)Skipped
Check Toml...............................................................Passed
Check Yaml...............................................................Passed
Check for merge conflicts................................................Passed
Check Yaml...............................................................Passed
Mixed line ending........................................................Passed
Fix requirements.txt.....................................................Passed
black....................................................................Failed
- hook id: black
- exit code: 1

Traceback (most recent call last):
  File "<removed>/py_env-python3.9/bin/black", line 8, in <module>
    sys.exit(patched_main())
  File "<removed>/py_env-python3.9/lib/python3.9/site-packages/black/__init__.py", line 1423, in patched_main
    patch_click()
  File "<removed>/py_env-python3.9/lib/python3.9/site-packages/black/__init__.py", line 1409, in patch_click
    from click import _unicodefun
ImportError: cannot import name '_unicodefun' from 'click' (<removed>/py_env-python3.9/lib/python3.9/site-packages/click/__init__.py)

autoflake................................................................Passed
flake8...................................................................Passed
markdownlint-fix.........................................................Passed
prettier.................................................................Passed
```
markbandstra: Maybe a Python3.9 issue? That's a commonality between my machine and the failing test runner.
jccurtis: @markbandstra I think so
jccurtis: This also occurs on `python3.10` with the same steps
jvavrek: > I just upgraded to click 8.0.4 (latest available on `conda-forge`) and I still don't see the error.

Scratch this, my `pre-commit` was skipping a bunch of stuff. I can replicate the error with
- Python 3.8.10
- black, 22.1.0 (compiled: no)
- pre-commit 2.12.0
- click 8.0.1
jccurtis: FYI This is supposedly fixed but I do not see the fix with the most updated packages: https://github.com/psf/black/pull/2966

```txt
❯ pip freeze
algopy==0.5.7
asteval==0.9.26
attrs==21.4.0
beautifulsoup4==4.10.0
-e git+https://github.com/lbl-anp/becquerel.git@62ddabd2db84d1e67fc1ad1f80433e13f0785b39#egg=becquerel
black==22.3.0
bump2version==1.0.1
certifi==2020.6.20
cfgv==3.3.1
charset-normalizer==2.0.12
click==8.1.0
coverage==6.3.2
cycler==0.11.0
distlib==0.3.4
filelock==3.6.0
flake8==4.0.1
fonttools==4.31.2
future==0.18.2
h5py==3.6.0
html5lib==1.1
identify==2.4.12
idna==3.3
iminuit==2.11.2
iniconfig==1.1.1
kiwisolver==1.4.2
llvmlite==0.38.0
lmfit==1.0.3
lxml==4.8.0
matplotlib==3.5.1
mccabe==0.6.1
mypy-extensions==0.4.3
nodeenv==1.6.0
numba==0.55.1
numdifftools==0.9.40
numpy==1.21.5
packaging==21.3
pandas==1.4.1
pathspec==0.9.0
patsy==0.5.2
Pillow==9.0.1
platformdirs==2.5.1
pluggy==1.0.0
pre-commit==2.17.0
py==1.11.0
pycodestyle==2.8.0
pyflakes==2.4.0
pyparsing==3.0.7
pytest==6.2.5
pytest-black==0.3.12
pytest-cov==3.0.0
pytest-rerunfailures==10.2
python-dateutil==2.8.2
pytz==2022.1
PyYAML==6.0
requests==2.27.1
scipy==1.8.0
six==1.16.0
soupsieve==2.3.1
statsmodels==0.13.2
toml==0.10.2
tomli==2.0.1
uncertainties==3.1.6
urllib3==1.26.9
virtualenv==20.14.0
webencodings==0.5.1
```
jvavrek: Comments in that psf/black link suggest 8.0.1 should work, so it's strange that I'm still seeing it with click==8.0.1
jccurtis: @jvavrek yep I see this with `click==8.0.1` and `click==8.0.4`. For reference:

```txt
black==22.3.0
pre-commit==2.17.0
```

(same as the `pip freeze` from above)
micahfolsom: Not sure what's going on there - bumping black to 22.3 fixed the issue for me, both locally, and in the pre-commit config.
micahfolsom: @jccurtis Not sure what's going on there - bumping black to 22.3 fixed the issue for me, both locally, and in the pre-commit config.
jccurtis: @micahfolsom beat me to it ... I was writing the PR 🤣: #329

The issue in the testing above was a bit of a 🤦 ... we weren't updating the `pre-commit` config
jccurtis: @micahfolsom let's get #329 in first. It makes the changelog easier to read 😄 
markbandstra: Great find @micahfolsom !

Patch:
diff --git a/becquerel/tools/materials_compendium.py b/becquerel/tools/materials_compendium.py
index af5988b..0e2af69 100644
--- a/becquerel/tools/materials_compendium.py
+++ b/becquerel/tools/materials_compendium.py
@@ -18,7 +18,7 @@ import os
 import warnings
 import numpy as np
 import pandas as pd
-from .materials_error import MaterialsWarning
+from .materials_error import MaterialsWarning, MaterialsError
 
 FNAME = os.path.join(os.path.split(__file__)[0], "MaterialsCompendium.json")
 
@@ -60,6 +60,23 @@ def fetch_compendium_data():
             data = json.load(f)
 
     # extract relevant data
+    if isinstance(data, list):
+        print("Pre-March 2022 JSON detected")
+    elif isinstance(data, dict):
+        print("Post-March 2022 JSON detected")
+        if "siteVersion" not in data.keys() or "data" not in data.keys():
+            raise MaterialsError(
+                "Attempt to read Compendium JSON failed; "
+                "dictionary must have keys 'siteVersion' "
+                "and 'data' but have keys " + str(list(data.keys()))
+            )
+        print(f"Compendium data - site version: {data['siteVersion']}")
+        data = data["data"]
+    else:
+        raise MaterialsError(
+            "Attempt to read Compendium JSON failed; "
+            "object must be a list or dict but is a " + str(type(data))
+        )
     names = [datum["Name"] for datum in data]
     formulae = [datum["Formula"] if "Formula" in datum else "-" for datum in data]
     densities = [datum["Density"] for datum in data]


Test patch:
diff --git a/tests/materials_test.py b/tests/materials_test.py
index ced6db6..6fe3a4f 100644
--- a/tests/materials_test.py
+++ b/tests/materials_test.py
@@ -87,8 +87,11 @@ def test_materials_dummy_csv():
 
 
 @pytest.mark.webtest
-def test_materials_dummy_compendium():
-    """Test fetch_materials with a dummy Compendium JSON file."""
+def test_materials_dummy_compendium_pre2022():
+    """Test fetch_materials with a dummy Compendium JSON file.
+
+    The dummy JSON file uses the format seen prior to March 2022.
+    """
     # point to an generate a dummy JSON file
     fname_orig = materials_compendium.FNAME
     materials_compendium.FNAME = fname_orig[:-5] + "_dummy.json"
@@ -128,6 +131,79 @@ def test_materials_dummy_compendium():
     materials_compendium.FNAME = fname_orig
 
 
+@pytest.mark.webtest
+def test_materials_dummy_compendium_2022():
+    """Test fetch_materials with a dummy Compendium JSON file.
+
+    The dummy JSON file uses the format first seen in March 2022.
+    """
+    # point to an generate a dummy JSON file
+    fname_orig = materials_compendium.FNAME
+    materials_compendium.FNAME = fname_orig[:-5] + "_dummy.json"
+    data = {
+        "siteVersion": "0.0.0",
+        "data": [
+            {
+                "Density": 8.4e-5,
+                "Elements": [
+                    {
+                        "AtomFraction_whole": 1.0,
+                        "Element": "H",
+                        "WeightFraction_whole": 1.0,
+                    }
+                ],
+                "Formula": "H2",
+                "Name": "Hydrogen",
+            },
+            {
+                "Density": 1.16e-3,
+                "Elements": [
+                    {
+                        "AtomFraction_whole": 1.0,
+                        "Element": "N",
+                        "WeightFraction_whole": 1.0,
+                    }
+                ],
+                "Formula": "N2",
+                "Name": "Nitrogen",
+            },
+        ],
+    }
+    with open(materials_compendium.FNAME, "w") as f:
+        json.dump(data, f, indent=4)
+    with pytest.warns(None) as record:
+        materials._load_and_compile_materials()
+    assert len(record) == 0, "Expected no MaterialsWarnings to be raised"
+    # remove siteVersion and make sure there is an error raised
+    del data["siteVersion"]
+    with open(materials_compendium.FNAME, "w") as f:
+        json.dump(data, f, indent=4)
+    with pytest.raises(MaterialsError):
+        materials._load_and_compile_materials()
+    # remove the dummy file and point back to original
+    os.remove(materials_compendium.FNAME)
+    materials_compendium.FNAME = fname_orig
+
+
+@pytest.mark.webtest
+def test_materials_dummy_compendium_error():
+    """Test fetch_materials with a dummy Compendium JSON file.
+
+    The dummy JSON file returns something that is not a list or dict.
+    """
+    # point to an generate a dummy JSON file
+    fname_orig = materials_compendium.FNAME
+    materials_compendium.FNAME = fname_orig[:-5] + "_dummy.json"
+    data = None
+    with open(materials_compendium.FNAME, "w") as f:
+        json.dump(data, f, indent=4)
+    with pytest.raises(MaterialsError):
+        materials._load_and_compile_materials()
+    # remove the dummy file and point back to original
+    os.remove(materials_compendium.FNAME)
+    materials_compendium.FNAME = fname_orig
+
+
 @pytest.mark.webtest
 def test_materials_no_compendium():
     """Test fetch_materials with no Compendium JSON file."""