DM-42285: Update to support numpy 1.25+ and various optimizations. #39
Conversation
This should probably be reverted in the future but at the moment this smooths any transitions.
Update for use with numpy 1.25/1.26
| @@ -382,7 +382,7 @@ def computeCCDChromaticity(self): | |||
| self.fgcmLog.warning("Found out-of-bounds value for chromaticity for filter %s, detector %d." % (self.fgcmPars.lutFilterNames[self.fInd], self.cInd + self.ccdStartIndex)) | |||
| continue | |||
|
|
|||
| self.fgcmPars.compCCDChromaticity[self.cInd, self.fInd] = res.x | |||
| self.fgcmPars.compCCDChromaticity[self.cInd, self.fInd] = res.x[0] | |||
There was a problem hiding this comment.
Do you also need to add the [0] to the if res.x <= -0.99 or res.x >= 0.99: access above?
There was a problem hiding this comment.
No, that's fine. The problem is that numpy doesn't like you setting one value with an array of length 1 anymore. Not sure why, but it doesn't like it. Comparisons to an array of length 1 are fine.
| @@ -86,6 +86,37 @@ def getMemoryString(location): | |||
|
|
|||
| return memoryString | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Add a ticket & TODO given the commit message?
There was a problem hiding this comment.
It's a tricky synchronization issue... and it won't hurt anything ... but I probably should file a ticket somewhere.
| @@ -10,6 +10,8 @@ | |||
| from .sharedNumpyMemManager import SharedNumpyMemManager as snmm | |||
|
|
|||
| from .fgcmUtilities import retrievalFlagDict | |||
| from .fgcmUtilities import histogram_rev_sorted | |||
|
|
|||
There was a problem hiding this comment.
Move these above from .sharedNumpyMemManager import SharedNumpyMemManager as snmm like the rest?
| np.add.at(expGrayRMSForInitialSelection, | ||
| obsExpIndex[goodObs], | ||
| EGray[goodObs]**2.) | ||
| (EGray[goodObs]).astype(expGrayRMSForInitialSelection.dtype)**2.) |
There was a problem hiding this comment.
Should the astype also wrap the **2. here?
There was a problem hiding this comment.
The opposite, I think. It should be promoted before squaring.
There was a problem hiding this comment.
Ok...you may want to look for other cases (I think I saw others where the promotion was after squaring).
| @@ -1221,7 +1221,7 @@ def _chisqWorker(self, goodStarsAndObs): | |||
| add_at_1d(partialArray[self.fgcmPars.parRetrievedLnPwvNightlyOffsetLoc: | |||
| (self.fgcmPars.parRetrievedLnPwvNightlyOffsetLoc+ | |||
| self.fgcmPars.nCampaignNights)], | |||
| 2.0 * deltaMagWeightedGOF[hasRetrievedPwvGOF] * innerTermGOF[hasRetrievedPwvGOF]) | |||
| (2.0 * deltaMagWeightedGOF[hasRetrievedPwvGOF] * innerTermGOF[hasRetrievedPwvGOF]).astype(np.float64)) | |||
| @@ -522,10 +522,10 @@ def _starWorker(self, goodStarsAndObs): | |||
|
|
|||
| np.add.at(objDeltaAperMeanTemp, | |||
| (obsObjIDIndex[goodObs], obsBandIndex[goodObs]), | |||
| (obsDeltaAper[goodObs] - self.fgcmPars.compMedDeltaAper[obsExpIndex[goodObs]])/obsMagErr2GO) | |||
| ((obsDeltaAper[goodObs] - self.fgcmPars.compMedDeltaAper[obsExpIndex[goodObs]])/obsMagErr2GO).astype(objDeltaAperMeanTemp.dtype)) | |||
There was a problem hiding this comment.
Oh, it may have just looked funny on my screen. Ignore!
| np.add.at(expGrayRMSColorSplit[:, c], | ||
| obsExpIndex[goodObs[use]], | ||
| EGrayGO[use]**2. / EGrayErr2GO[use]) | ||
| (EGrayGO[use]**2. / EGrayErr2GO[use]).astype(expGrayRMSColorSplit.dtype)) | ||
| np.add.at(expGrayNGoodStarsColorSplit[:, c], | ||
| obsExpIndex[goodObs[use]], | ||
| 1) |
| np.add.at(superStarOffset, | ||
| (self.fgcmPars.expEpochIndex[obsExpIndex[goodObs2]], | ||
| self.fgcmPars.expLUTFilterIndex[obsExpIndex[goodObs2]], | ||
| obsCCDIndex[goodObs2]), | ||
| EGrayGO[mark]/EGrayErr2GO[mark]) | ||
| (EGrayGO[mark]/EGrayErr2GO[mark]).astype(superStarOffset.dtype)) | ||
| np.add.at(superStarNGoodStars, | ||
| (self.fgcmPars.expEpochIndex[obsExpIndex[goodObs2]], | ||
| self.fgcmPars.expLUTFilterIndex[obsExpIndex[goodObs2]], |
There was a problem hiding this comment.
Not a problem here.
| @@ -1,6 +1,6 @@ | |||
| try: | |||
| from numba import jit | |||
| has_numba = True | |||
| has_numba = False | |||
| except ImportError: | |||
There was a problem hiding this comment.
Commit message is long. Could do:
Turn off numba usage since it seems to be slower
This is especially evident with new (i.e. version 1.25+) numpy.
There was a problem hiding this comment.
Commit is already pushed to fgcm master and released so ...
| @@ -1,6 +1,6 @@ | |||
| try: | |||
| from numba import jit | |||
| has_numba = True | |||
| has_numba = False | |||
There was a problem hiding this comment.
In case I bring this code back in the future, I guess.
There was a problem hiding this comment.
This doesn't really facilitate that (and I'd argue it's more confusing to leave it like this), but up to you!
This also updates to ensure that reverse indices are sorted. This is important to ensure reserved stars and such are consistent from system to system and numpy version to numpy version.
Upstream problem reported here: esheldon/esutil#87
Apparently this has been "loose" forever but it just happened to work okay until now.
Using this branch and numpy 1.24 the fgcmcal tests are 5% faster (and this is a lower limit on the speed improvement in larger dataset). Using this branch and numpy 1.26 the fgcmcal tests are another 5% faster still.