Include zscale #28
Include zscale #28
Conversation
There was a problem hiding this comment.
Hi, thanks for your contribution! Besides a few comments I wrote down (only the first time I found a specific issue, but they're repeated several times across this file), however there are many things to improve:
- there are no docstrings and no references for what you're doing, it's really hard to follow what you do if you don't explain it
- there are no tests anywhere
- this code isn't actually used anywhere
| MAX_REJECT = 0.5 | ||
| MIN_NPIXELS = 5 | ||
| GOOD_PIXEL = 0 | ||
| BAD_PIXEL = 1 | ||
| KREJ = 2.5 | ||
| MAX_ITERATIONS = 5 |
There was a problem hiding this comment.
Please, have a read of the performance tips in the Julia manual: https://docs.julialang.org/en/v1/manual/performance-tips/. Not using non-constant global variables is the very first issue. Some of these variables aren't even used in more than a function, so making them global isn't even useful.
| function flatten(image) | ||
|
|
||
| # colapses into one dimension (Float64) a 2D array in row-major (C-style flattening) | ||
|
|
||
| rows = size(image)[1] | ||
| cols = size(image)[2] | ||
| n_pxls = rows * cols | ||
|
|
||
| result = Array{Float64}(undef, n_pxls) | ||
|
|
||
| k = 1 | ||
| for i in 1:rows, j in 1:cols | ||
| result[k] = image[i, j] | ||
| k += 1 | ||
| end | ||
|
|
||
| return result | ||
| end |
There was a problem hiding this comment.
This is what image[:] does, this function isn't needed
| nl = size(image)[2] | ||
|
|
||
| stride = maximum([1.0, sqrt((nc - 1) * (nl - 1) / maxpix)]) | ||
| stride = Int128(round(stride)) |
| sumz = 0 | ||
| sumsq = 0 |
There was a problem hiding this comment.
These variables will likely be type-unstable
| end | ||
|
|
||
|
|
||
| function zsc_sample(image, maxpix) |
There was a problem hiding this comment.
What is this function actually doing and how? Please add docstrings to functions.
| end | ||
|
|
||
|
|
||
| function zsc_compute_sigma(flat, badpix) |
There was a problem hiding this comment.
Also here (and everywhere) a docstring would be useful. Please also explain the meaning of the mean or sigma equal to nothing (which I'm not sure is a good idea)
|
|
||
| function zsc_fit_line(samples, npix, krej, ngrow, maxiter) | ||
|
|
||
| # calculate slope and intercept for the algorithm |
There was a problem hiding this comment.
Which algorithm? There is no reference to what you're doing here.
| center_pixel = Int128(round((npix + 1) / 2)) | ||
| median = samples[center_pixel] | ||
| else | ||
| Int128(round(npix / 2)) |
There was a problem hiding this comment.
It is supposed to be:
center_pixel = Int128(round(npix / 2))
Will rectify it with other changes.
|
Okay thank you for such a detailed response; I'll begin working on your suggestions. |
|
@Devagio I've coded zscale in Julia over at PlotUtils.jl, you can use that as a reference if you like. It's been a while since I wrote that code, you may find ways to improve upon it. Otherwise you may wish to just copy it, this decouples any reliance on the Plots.jl-verse. |
|
Given the fact we're going towards #29, I'm not sure we really need to add a zscale implementation here. |
|
With regards to #29, right now I have it re-exporting zscale from PlotUtils so that one can pass |
This DS9/IRAF feature allows defining the image scale according to IRAF's z-scale and returns a minimum and maximum color. This fixes #27