Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-21141

ip_diffim breaks with numpy 1.17

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: ip_diffim
    • Labels:
      None
    • Team:
      External

      Description

      Sogo Mineo reports on installing the LSST stack with numpy 1.17:

      This line needs modifying https://github.com/lsst/ip_diffim/blob/master/python/lsst/ip/diffim/diffimTools.py#L88 as:

      -intNoiseArr = rand.poisson(imArr)
      +intNoiseArr = rand.poisson(np.where(np.isfinite(imArr), imArr, 0.0))
      

      in order for ip_diffim's tests to pass. I think this is because I am using numpy 1.17, in which numpy.random changed largely. rand.poisson() no longer accepts lam >= 9.223372006484772e+18 or lam = nan.

        Attachments

          Activity

          Hide
          price Paul Price added a comment -

          Gabor Kovacs, I'm going to guess that this is your package these days. If not, would you be able to suggest an alternative reviewer, please?

          Show
          price Paul Price added a comment - Gabor Kovacs , I'm going to guess that this is your package these days. If not, would you be able to suggest an alternative reviewer, please?
          Hide
          gkovacs Gabor Kovacs added a comment -

          Gabor Kovacs, I'm going to guess that this is your package these days. If not, would you be able to suggest an alternative reviewer, please?

          I've been out of office; of course, I can do the review.

          Show
          gkovacs Gabor Kovacs added a comment - Gabor Kovacs , I'm going to guess that this is your package these days. If not, would you be able to suggest an alternative reviewer, please? I've been out of office; of course, I can do the review.
          Hide
          tjenness Tim Jenness added a comment -

          Brian Van Klaveren is this patch sufficient for you or did you need more to get your build working with new numpy?

          Show
          tjenness Tim Jenness added a comment - Brian Van Klaveren is this patch sufficient for you or did you need more to get your build working with new numpy?
          Hide
          gkovacs Gabor Kovacs added a comment -

          Paul Price Could you please provide some instructions how to demonstrate the original problem? What I tried:

          • a recent lsstsw build in a "normal" environment, then pip install -U numpy to v 1.17.2 in a venv and trying to rebuild ip_diffim separately. Actually the tests have passed with a bunch of deprecation warnings and a warning of possible binary numpy incompatibility (version mismatch with other components).
          • pip install -U numpy right after the deploy / setup scripts and rebuilding the whole stack. Then rebuild fails in astro_metadata_translator with astropy quantity error.

          The code change is obviously OK, I think it would be worth of pointing to the changelog or docs in this ticket that mention the stated numpy.random changes; I could not trivially link the 1.17 numpy changelog to this.

          Show
          gkovacs Gabor Kovacs added a comment - Paul Price Could you please provide some instructions how to demonstrate the original problem? What I tried: a recent lsstsw build in a "normal" environment, then pip install -U numpy to v 1.17.2 in a venv and trying to rebuild ip_diffim separately. Actually the tests have passed with a bunch of deprecation warnings and a warning of possible binary numpy incompatibility (version mismatch with other components). pip install -U numpy right after the deploy / setup scripts and rebuilding the whole stack. Then rebuild fails in astro_metadata_translator with astropy quantity error. The code change is obviously OK, I think it would be worth of pointing to the changelog or docs in this ticket that mention the stated  numpy.random changes; I could not trivially link the 1.17 numpy changelog to this.
          Hide
          sogo.mineo Sogo Mineo added a comment -

          I encountered this problem when I was installing the lsst stack not with anaconda but with gcc-9.2, python-3.7, and pip for all prerequisite python modules. If numpy-1.17 with the normal environment works, I suspect that my installation was indeed broken, that something wrong caused non-finite values in imArr, and that the test code detected the error accurately.

          Show
          sogo.mineo Sogo Mineo added a comment - I encountered this problem when I was installing the lsst stack not with anaconda but with gcc-9.2, python-3.7, and pip for all prerequisite python modules. If numpy-1.17 with the normal environment works, I suspect that my installation was indeed broken, that something wrong caused non-finite values in imArr , and that the test code detected the error accurately.
          Hide
          gkovacs Gabor Kovacs added a comment -

          Brian Van Klaveren 16:05

          Before I pulled in those commits, I had 11 tests fail. After I had pulled in those commits, and I was still having some errors (5). Today I re-tested and the other 5 errors are gone :man-shrugging: I looked at the previous environment and it says 1.17.2 too, so I don't know (edited)

           

          Following up on slack messaging, it seems to be unclear which combination of environment components caused the problem, anyway, I am fine with this change, it makes the code safer.

          Show
          gkovacs Gabor Kovacs added a comment - Brian Van Klaveren 16:05 Before I pulled in those commits, I had 11 tests fail. After I had pulled in those commits, and I was still having some errors (5). Today I re-tested and the other 5 errors are gone :man-shrugging: I looked at the previous environment and it says 1.17.2 too, so I don't know (edited)   Following up on slack messaging, it seems to be unclear which combination of environment components caused the problem, anyway, I am fine with this change, it makes the code safer.
          Hide
          price Paul Price added a comment -

          Thanks Gabor.

          Merged to master.

          Show
          price Paul Price added a comment - Thanks Gabor. Merged to master.

            People

            • Assignee:
              price Paul Price
              Reporter:
              price Paul Price
              Reviewers:
              Gabor Kovacs
              Watchers:
              Gabor Kovacs, Paul Price, Sogo Mineo, Tim Jenness
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel