Mosfit not fully compatible with astropy v5
robertdstein opened this issue · 7 comments
Some features of Mosfit are not compatible with astropy 5, because astropy Quantity values are no longer directly compatible with numpy.logspace
(see e.g astropy/astropy#10573).
A minimal example, with astropy==4.3.1
and numpy=1.22
:
mosfit -m slsn
which runs without errors. However, repeating the same command with astropy==5.0
and numpy=1.22
yields:
File "/home/rdstein/.conda/envs/tdescore_env/bin/mosfit", line 8, in <module>
sys.exit(main())
File "/home/rdstein/.conda/envs/tdescore_env/lib/python3.8/site-packages/mosfit/main.py", line 973, in main
Fitter(**fitargs).fit_events(**fitargs)
File "/home/rdstein/.conda/envs/tdescore_env/lib/python3.8/site-packages/mosfit/fitter.py", line 477, in fit_events
entry, p, lnprob = self.fit_data(
File "/home/rdstein/.conda/envs/tdescore_env/lib/python3.8/site-packages/mosfit/fitter.py", line 686, in fit_data
output = model.run_stack(x, root='output')
File "/home/rdstein/.conda/envs/tdescore_env/lib/python3.8/site-packages/mosfit/model.py", line 970, in run_stack
new_outs = self._modules[task].process(**inputs)
File "/data/rdstein/.conda/envs/tdescore_env/lib/python3.8/site-packages/mosfit/modules/arrays/densetimes.py", line 36, in process
x + self._rest_t_explosion for x in np.logspace(
File "<__array_function__ internals>", line 180, in logspace
TypeError: no implementation found for 'numpy.logspace' on types that implement __array_function__: [<class 'astropy.units.quantity.Quantity'>]
I am pretty sure this is the same problem problem described in #207.
I think this problem would be rather complex to solve. It would involve ensuring that all variables passed in Mosfit are either always an astropy quantity or always a numpy float/int etc. Right now some variables can swap between the categories. An example is self._rest_t_explosion
in densetimes.py
, which causes the error above and is both an astropy quantity and numpy float at different points during the call of mosfit -m slsn
. If the variables were reliably in one class or the other, the astropy quantities would then need to be converted each time np.logspace
are used.
For
you can't simply replace withnp.log10(max_times - self._rest_t_explosion.value)
because if self._rest_t_explosion
is not an astropy quantity this will throw an error.I've finally got to testing this, but I can't reproduce this error or the related #207. Which version of Python are you using? If you try with Python 3.10, astropy 5.1 and numpy 1.23, do you still get an error?
I tried with the latest github version of Mosfit (1.1.8), python 3.10.4, astropy 5.1, numpy 1.23.3, and I do still get the error. For reference this is on a Mac M1 (2021).
I then tried the a clean install on a random linux machine:
- create a new conda environment, and activate it
- conda install -c conda-forge mpi4py
- pip install mosfit
- mosfit -m slsn
and there I get exactly the same error. So it does not seem to be platform-specific for me. Anything obviously wrong there?
We're now seeing this error in the conda-forge feedstock build process: conda-forge/mosfit-feedstock#50 . Current builds are with Astropy 5.1.1, Numpy 1.23.4, across Python versions.
I believe I have now fixed this!
I'm bumping the version number so we can update the distributions. I've done this on pypi -- Peter would you be able to do it on conda-forge? I'm not sure of the process for that.
Would also be great to get confirmation that this is working for others
@mnicholl Great! The conda-forge update should be close to automatic — we have bots that look for new releases and automatically submit pull requests to initiate the updates. I'll ping you if anything comes up but you basically shouldn't have to worry about that part of the process.
I confirm that I just tested this with astropy 5.1.1. The error is reproduced for mosfit 1.1.8, but is completely solved in 1.1.9, so it looks like the fix works. Thanks @mnicholl!
As seen in conda-forge/mosfit-feedstock#51, the new release builds successfully in conda-forge too. I think it's safe to mark this one as solved.