skypyproject / skypy

SkyPy: A package for modelling the Universe.
BSD 3-Clause "New" or "Revised" License
118 stars 38 forks source link

BUG: m_min and m_max astropy quantity error #503

Open Fox-Davidson opened 2 years ago

Fox-Davidson commented 2 years ago

Describe the bug When working in the skypy\halos module, the maximum and minimum mass (variable names m_min and m_max) to draw from the halo mass function are converted to astropy quantities before any further functions are called.

To Reproduce Steps to reproduce the behavior:

  1. Code tested using the function "colossus_mf" in skypy\halos\ _colossus.py through a configuration file
  2. "colossus_mf" calls functions "colossus_mf_redshift" and "colossus_mass_sampler"
  3. No issue in "colossus_mf_redshift" despite using "lnmmin = np.log(m_min*cosmology.h)"
  4. Issue in "colossus_mass_sampler" line "m_h0 = np.logspace(np.log10(m_min h0), np.log10(m_max h0), resolution)" producing error "TypeError: no implementation found for 'numpy.logspace' on types that implement __ array_function __: [<class 'astropy.units.quantity.Quantity'>]"

Expected behavior m_min and m_max should be floats not <class 'astropy.units.quantity.Quantity'> and hence not produce the error. This can therefore be temporarily fixed by using "m_min.value" instead.

Desktop (please complete the following information):

Additional context Adding debug print statements in "colossus_mf" but before "colossus_mf_redshift" is called (first line of function) shows that it is already an Astropy quantity but nothing could be found in the pipeline files that would do this. The only decorator is for the sky area (@units.quantity_input(sky_area=units.sr)) and m_min/m_max are passed as floats in the config file. halo_test.yml.txt

rrjbca commented 2 years ago

@katie-davidson I cannot reproduce the error you are seeing without a minimum working example; please provide a simple config file or python script. However, I note that as currently implemented the function colossus_mf expects m_min and m_max to be floats; see the documentation here. If you are trying to pass astropy quantities for these arguments that could be causing the issue.

Fox-Davidson commented 2 years ago

Sorry @rrjbca I have added a config file now. As far as I can work out I pass them in as floats but by the time they get to the colossus_mf function, they are already Astropy units and I don't know when this happens.

rrjbca commented 2 years ago

TLDR: replace the relevant lines in your config with:

  m_min: 1.e+9
  m_max: 1.e+12

In YAML 1.1 1e+9 and 1e+12 are not valid scientific notation so these entries in the config file do not get parsed as floats. But according to astropy they are valid scientific notation, so they get parsed as dimensionless astropy quantities. The problem is that the logspace function isn't implemented to take astropy quantities, even when they are dimensionless i.e. no units. The simplest fix is to therefore add the decimal point to the values in your config so that pyyaml parses them as floats. More generally we could consider the following changes to the code:

Tagging @Lucia-Fonseca and @ntessore for input.

Fox-Davidson commented 2 years ago

Thanks, I added the dots in the config file and removed the .value from the code and it all works now. Does this count as closed or should I leave it open for the further actions mentioned above?