filter_by_redshift_range does not work (for 'ionized_fraction')

Stefan-Heimersheim commented 4 years ago

Hi,

the filter_by_redshift_range doesn't seem to work anymore for some fields, for example:

import corecon as crc
crc.filter_by_redshift_range('ionized_fraction', 1, 10)

returns

Traceback (most recent call last):
  File "bug.py", line 2, in <module>
    crc.filter_by_redshift_range('ionized_fraction', 1, 10)
  File "/home/stefan/.local/lib/python3.8/site-packages/corecon/__init__.py", line 282, in filter_by_redshift_range
    values                 = d[k].values[w],
IndexError: too many indices for array

When calling it with an interval where there are no measurements, e.g. crc.filter_by_redshift_range('ionized_fraction', 100, 101), it happily returns an empty array, so the error only occurs when there are actually measurements.

Another improvement to that function I would suggest is giving an error message when the user swaps zmin and zmax, e.g. crc.filter_by_redshift_range('ionized_fraction', 10, 1). It currently returns an empty dictionary. I would suggest something along the lines of

assert zmin<=zmax, "zmax cannot be smaller than zmin, you provided zmin={0:.2f}, zmax={1:.2f}".format(zmin, xmax)

Cheers, Stefan

EGaraldi commented 2 years ago

Thanks, I've now fixed this issue (hopefully, it was a bit obscure why), and added the assertion

Stefan-Heimersheim commented 1 year ago

Hi! I've just been asked by @afialkov to check whether the changes from the review had been implemented. I've just installed the new version of the code (0.6.1) and tried to reproduce the command above, I believe the new syntax is

import corecon as crc
crc.get_field("ionized_fraction").filter_by_redshift_range(1, 10)

This appears to fail with, am I doing this correctly?

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/stefan/_software/corecon/corecon/FieldClass.py", line 107, in filter_by_redshift_range
    if any(w):
       ^^^^^^
TypeError: 'numpy.bool_' object is not iterable

This does not seem to be essential functionality though

EGaraldi commented 1 year ago

Hi, thanks for the feedback. It seems a version issue, since it works for me with python 3.7.7, but indeed and numpy 1.21.5, but it seems to break for python 3.9 and 3.10 with numpy 1.24. Can you share the version of python and numpy you are using?

EGaraldi commented 1 year ago

Nevermind, I figured it out. I pushed an update to the repo, and I will include in the next release

EGaraldi / corecon

filter_by_redshift_range does not work (for 'ionized_fraction') #5