query_disc bug

[bwinkel]

Under rare circumstances query_disc produces wrong results. This seems to happen, when the
sphere edge "touches" the equator (but not always).

import healpy as hp
import numpy as np
import matplotlib.pyplot as plt

radius = 0.1
hpxidx = hp.query_disc(256, hp.ang2vec(np.pi/2 - radius, np.pi), radius, nest=False)
theta, phi = hp.pix2ang(256, hpxidx, nest=False)

plt.plot(phi, np.pi/2 - theta, 'x')
plt.xlabel(r'$\phi\,[\mathrm{rad}]$', fontsize=14)
plt.ylabel(r'$\pi/2 - \theta\,[\mathrm{rad}]$', fontsize=14)
plt.show()

[mreineck]

This is most likely a bug in the C++ library. I'll try to come up with a fix soon!

[mreineck]

I think I have a fix. Is it sufficient if I commit this to the Healpix SVN trunk, or do you need it on a specific branch as well, Andrea?

[zonca]

Trunk is fine
On Mar 17, 2015 6:40 AM, "mreineck" notifications@github.com wrote:

I think I have a fix. Is it sufficient if I commit this to the Healpix SVN
trunk, or do you need it on a specific branch as well, Andrea?

—
Reply to this email directly or view it on GitHub
#229 (comment).

[mreineck]

Ok, it's there already.

[lpsinger]

@mreineck, would you please explain what you changed in r683?

[mreineck]

Very hard to describe ... the original code always assumed that the whole ring that's currently being processed would lie inside the queried disk if no intersection points could be determined between ring and disk. In the case of a non-inclusive query this could be incorrect, and "no intersection" could in fact mean "ring lies completely outside the disk". I tried to address that problem.

[lpsinger]

Interesting. Are you going to add a test case to healpix-cxx?

[mreineck]

I'm thinking about adding the test case above to hpxtest; but a positive result will not necessarily indicate that the algorithm works as expected - it could also just happen to work correctly because of numerical noise. If I can think of a more stringent test, I'll use that!

[lpsinger]

I don't think it worked... I checked out the latest code from trunk, rebuilt, and ran @bwinkel's test script. The result is attached below. What am I even looking at here?

[bwinkel]

Looks fine to me. It's the result of the query_disc plotted in lon/lat coordinates (radians).

[lpsinger]

Oh! I just realized that those are crosses marking points in a scatter plot; I had thought that they were thin stripes across a HEALPix image.

@mreineck, when is your next opportunity for a healpix-cxx release? It would be great to get this bug fix into both the standalone packages and the source that we are bundling with healpy.

[mreineck]

Sorry, that took some time ...
I can prepare a new autotools-generated tarball, and upload it to SourceForge. The small problem with this release is that it breaks backwards compatibility not with respect to the library, but with respect to the delivered binaries: the original "alice" code is gone and will be replaced by "alice3". Should this be reflected somehow in the version number? If yes, this might start another political discussion ...

[zonca]

ok, I updated the healpix git mirror:

healpy/healpixmirror@0422b3a

should I update the healpy git submodule or should we wait for a new
healpix release?

On Mon, Mar 23, 2015 at 3:56 AM, mreineck notifications@github.com wrote:

Sorry, that took some time ...
I can prepare a new autotools-generated tarball, and upload it to
SourceForge. The small problem with this release is that it breaks
backwards compatibility not with respect to the library, but with respect
to the delivered binaries: the original "alice" code is gone and will be
replaced by "alice3". Should this be reflected somehow in the version
number? If yes, this might start another political discussion ...

—
Reply to this email directly or view it on GitHub
#229 (comment).

[lpsinger]

The small problem with this release is that it breaks backwards compatibility not with respect to the library, but with respect to the delivered binaries: the original "alice" code is gone and will be replaced by "alice3". Should this be reflected somehow in the version number?

The Makefile.am file will have to be updated to create the new executable.

The new release should have a new version number, but it's up to you which part of the version number should be incremented.

[lpsinger]

should I update the healpy git submodule or should we wait for a new
healpix release?

Might as well coordinate the changes and simultaneously release new versions of healpix-cxx and healpy.

[lpsinger]

@mreineck, the last few lines of output from hpxtest are:

checking issue #229
checking issue #229
checking query_disc() (basetype: int)
  PROBLEM: RING pixel sets inconsistent
checking query_disc() (basetype: long)
  PROBLEM: RING pixel sets inconsistent
  PROBLEM: RING pixel sets inconsistent
  PROBLEM: RING pixel sets inconsistent
checking query_polygon() (basetype: int)
checking query_polygon() (basetype: long)

Is this related, and what does it mean? Does this constitute a unit test failure? If so, why doesn't hpxtest return a nonzero exit status?

[mreineck]

It's not (really) a bug and unrelated. Hpxtest isn't really a set of unit tests either ... it reports problems and errors on stdout and probably shouldn't run as part of "make distcheck" before it has been properly adjusted. Unfortunately I haven't found time for this so far.

[lpsinger]

It's not (really) a bug and unrelated.

Great!

Hpxtest isn't really a set of unit tests either ... it reports problems and errors on stdout and probably shouldn't run as part of "make distcheck" before it has been properly adjusted.

Just testing that it runs without crashing is helpful; we found some issues with OpenMP on extremely esoteric microcontrollers this way. I think we should leave it in the make check target for now.

[mreineck]

I just made the necessary change ... future releases of hpxtest will return an error status if a regression is detected.

[lpsinger]

Thank you, @mreineck! Can we add a C++ implementation of @bwinkel's test case too? Here is a quick translation (sorry for my rusty C++):

template<typename I>void check_query_disc_issue_229()
  {
  // check for regression of https://github.com/healpy/healpy/issues/229
  cout << "checking query_disc() healpy/healpy#229 " << bname<I>() << endl;
  int order = 8;
  double radius = 0.1;
  T_Healpix_Base<I> rbase (order,RING);
  rangeset<I> pixset;
  pointing ptg(M_PI_2 - radius, M_PI);
  vec3 p = ptg.to_vec3();
  rbase.query_disc(ptg,radius,pixset);
  vector<I> ipixes = pixset.toVector();
  for (int i = 0; i < ipixes.size(); i ++)
    {
    I ipix = ipixes[i];
    vec3 q = rbase.pix2vec(ipix);
    if (dotprod(p, q) < cos(radius))
      FAIL(cout << "  PROBLEM: regression of https://github.com/healpy/healpy/issues/229 at pixel " << ipix << endl);
    }
  }

One question:

#define FAIL(a) {a; if (++errcount>100) planck_fail("unit test errors");}

Does this mean that the tests fail only if there are 100 or more failures?

[mreineck]

The tst case is already in: see check_issue_229()

Concerning the FAIL: if more than 100 errors have accumulated, the code stops immediately. Otherwise the tests will run to the end, but then an error will be returned if (errcount>0). This is mostly for the benefit of people who run hpxtest interactively.

[lpsinger]

Awesome! This bug is most certainly dead now. @zonca, can we close this?

[zonca]

sure, but are we going to have an Healpix release?

[lpsinger]

sure, but are we going to have an Healpix release?

Yes please!