usnistgov / mass

Microcalorimeter pulse-analysis software
MIT License
6 stars 0 forks source link

Save and load recipe books for mass.off #279

Closed ggggggggg closed 5 months ago

ggggggggg commented 5 months ago

This PR gets saving and loading of recipe books working for mass.off. It makes two changes to support this, only one of which actually matters at the moment.

  1. replaces many usages of lambda with custom defined classes that live in recipe_classes.py
  2. Change the way coef_dtype is used to allow indexing with both filtValue and coefs

Regarding 1. In the past, I used dill instead of pickle to enable saving a labmdas. At some point this stopped working. Now it seems to work fine again. The saving and loading lambdas is listed as one of the "major features" of dill, so it sure seems like they intend it to work. Removing the lambdas makes the code a bit harder to read, but otherwise has little effect. I'm tempted to leave in this work even though it currently seems unnecessary. In case dill breaks again, we'll be halfway to working without dill. Currently there are still a few usages of lambdas in tests, but not in the main codebase.

The second change has to do with a feature I probably shouldn't have added, which is like a view in a database. Basically the same column of data can be indexed a few different ways. The way I did it before, the Recipe has a reference to an OffFile and therefore to a mmap. Now RecipeBook only stores a numpy.dtype, which is easily pickleable.

I added a new test file that uses 5lag filters with saving and loading of recipe books.

joefowler commented 5 months ago

Do these two warnings during tests worry you? This is what I see locally, but the GitHub Actions page shows the same.

===================================================================== warnings summary =====================================================================
tests/off/test_5lag_off.py::test_off_5lag_with_saving_and_loading_recipes
  /Users/fowlerj/qsp/lib/python3.11/site-packages/numpy/core/fromnumeric.py:3504: RuntimeWarning: Mean of empty slice.
    return _methods._mean(a, axis=axis, dtype=dtype,

tests/off/test_5lag_off.py::test_off_5lag_with_saving_and_loading_recipes
  /Users/fowlerj/qsp/lib/python3.11/site-packages/numpy/core/_methods.py:129: RuntimeWarning: invalid value encountered in divide
    ret = ret.dtype.type(ret / rcount)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
======================================================= 188 passed, 1 xfailed, 2 warnings in 35.80s ========================================================
joefowler commented 5 months ago

Don't think I 100% understand your first point. Do you mean that these changes removed most but not all uses of dill? That would be nice. But if they remove all uses, then let's dump that dependency. It's been a pain in the past.

joefowler commented 5 months ago

I made some updates, saving recipe pickles in temporary files. Seem reasonable?

ggggggggg commented 5 months ago

Don't think I 100% understand your first point. Do you mean that these changes removed most but not all uses of dill? That would be nice. But if they remove all uses, then let's dump that dependency. It's been a pain in the past.

Currently the main code (in mass/off) doesn't need dill, but some of the tests do need dill because they use lambdas. So if dill fails on us again, I think it will be easy to fix.