Conversation
jnation3406
left a comment
jnation3406
left a comment
There was a problem hiding this comment.
Looks fine, but I think it would be more efficient to do it with the whole array of flux/fluxerr into mag/magerr using numpys fast array operations, rather than doing it pixel by pixel.
jnation3406
left a comment
jnation3406
left a comment
There was a problem hiding this comment.
Better, but I think the method is confusing since you are trying to support different input/output types. I think it would be clearer to split it into two methods based on input/outputs
| if flux <= 0: | ||
| return None, None | ||
| flux_array = np.asarray(flux) | ||
| fluxerr_array = np.asarray(fluxerr) |
There was a problem hiding this comment.
Are these not already numpy arrays when pulled out of the fits file by astropy?
|
|
||
| mag = -conversion_factor * np.log10(flux_array) | ||
| magerr = flux2mag * (fluxerr_array / flux_array) | ||
| return float(mag), float(magerr) |
There was a problem hiding this comment.
I assume you are doing this to support both single values and numpy arrays of values. I think it might be worth breaking this out into two methods, since it returns different things depending on its input. This will be less confusing, and maybe simplify the logic of each individual method.
jnation3406
left a comment
jnation3406
left a comment
There was a problem hiding this comment.
Looks good. Pretty spiffy you have a function that works with both scalars and numpy arrays the same. Can you add a unit test for having negative flux values in a numpy array?
2 participants
Refactors
flux_to_magto be a util function and is used to returnmagandmag_errin the source_catalog