Tickets/dm 9229

[yalsayyad]

No description provided.

[natelust]

It may not be necessary, as this function is only called a small number of times, but it is possible to avoid all the overhead of calling a function, and the steps in the function by saying:
minPsfSize = int(np.ceil(self.KConfig.kernelSize*self.config.autoPadPsfTo))|1
This will return the number if it is odd, or the next integer if it is even

[yalsayyad]

I acknowledge two suggestions here: (1) to take the minPsfSize calculation out of a function and (2) to switch the last bit to 1 to get the next odd integer.
1: This function is not in a loop and only called once. My rationale for putting it in a function was to reduce code duplication with the unit test. If you argue that unit tests should calculate it differently, then I'll take it out of the function.
2: Flipping the last bit to 1 smells clever to me, because it doesn't read like english.

The function plus english-reading % 2 version also appears to be (counterintuitively) faster, but a microsecond (or even 100 milliseconds) won't make a difference in a long task like ModelPsfMatch:

import timeit

setupBitFlip = """
from random import random
import numpy as np
def nextOddInt(x):
    return int(np.ceil(x))|1

def genKRand():
    for i in range(1000):
        yield 10*random()
"""

setupIfMod = """
from random import random
import numpy as np

def genKRand():
    for i in range(1000):
        yield 10*random()

def nextOddInt(x):
    return x if x % 2 == 0 else x + 1
"""

print('%.02f us'% (1000*timeit.timeit('[nextOddInt(y) for y in genKRand()]', setup=setupIfMod, number=10)/10.))
# 0.63 us
print('%.02f us'% (1000*timeit.timeit('[nextOddInt(y) for y in genKRand()]', setup=setupBitFlip, number=10)/10.))
# 1.26 us
print('%.02f us'% (1000*timeit.timeit('[int(np.ceil(y))|1 for y in genKRand()]', setup=setupBitFlip, number=10)/10.))
# 1.07 us

# Single value
print('%.02f us'%(timeit.timeit('nextOddInt(3.4)', setup=setupIfMod, number=1000000)))
# 0.35 us
print('%.02f us'%(timeit.timeit('nextOddInt(3.4)', setup=setupBitFlip, number=1000000)))
# 0.99 us
print('%.02f us'%(timeit.timeit('int(np.ceil(3.4))|1', setup=setupBitFlip, number=1000000)))
# 0.82 us

[natelust]

The speed difference here comes from setupBitFlip using int(np.ceil( and setupIfMod not, it seems to just be doing the mod on whatever type that x is. Over all I don't have a strong feeling on this and reuse in the unit test is a good enough argument for me.

[yalsayyad]

Yeah, whoops. Let me update those last times for posterity:

# # Single value UPDATED
print('%.02f us'%(timeit.timeit('nextOddInt(3.4)', setup=setupIfMod, number=1000000)))
# 1.23 us
print('%.02f us'%(timeit.timeit('nextOddInt(3.4)', setup=setupBitFlip, number=1000000)))
# 1.02 us
print('%.02f us'%(timeit.timeit('int(np.ceil(3.4))|1', setup=setupBitFlip, number=1000000)))
# 0.87 us

[natelust]

please change the import to import numpy as np, as it is what the community has settled on, and it will make this file more consistent with the rest of the stack which uses that import.

[yalsayyad]

👍 (but on a separate commit because it is independent of this ticket)

[natelust]

remove either "a" or "the" and change + to "add" or "pad by" the sentence is a bit confusing on the first read. I read it as "If the Science Psf is smaller than (the matching kernel + a minimum fraction)"

[yalsayyad]

👍

[natelust]

Is it worth just moving to the next even number rather than throwing an error? Or maybe being explicit is good, like you have it. If the algorithm must have an even to work, perhaps move to the next even number and put out a warning? What you have is fine, these are just some things for you to consider.

[yalsayyad]

I considered that. My reasoning for raising an error is that when setting doAutoPadPsf=False, I presume the user has reasons for turning off the automatic tuning, and thus wants exact manual control over the configs and wants ModelPsfMatchTask to just do exactly what they ask of it or quit. The behavior is consistent: Kernel too big? Raise. Padding inappropriately odd? Raise. I suspect this config option will only be used when users want precise control such as for testing config parameters like you did for the HSC image

[natelust]

If you changed to the binary operator in the other file, remember to change where the function is used here as well