search

EoRImaging / FHD

Fast Holographic Deconvolution
BSD 2-Clause "Simplified" License
20 stars 10 forks source link

Repeated code in visibility_count #273

Closed nicholebarry closed 2 years ago

nicholebarry commented 2 years ago

The majority of code in visibility_count is actually a repeat of visibility_grid. This is bad news, because visibility_grid has since been updated and visibility_count has not!

The repeated code needs to be condensed into one function/procedure.

nicholebarry commented 2 years ago

Hmmm we could completely drop visibility_count. The same output is calculated in situ in visibility_grid and then saved as a vis_count file. Given the passed (or not passed) variables in the filter functions, visibility_count is never actually used, and we fully rely on the previously calculated vis_count file (related to what I was trying to account for in #272 ).

What do you think @isullivan, can we just drop visibility_count.pro altogether? We can pass the visibility count (uniform I think it's called coming out of visibility_grid) back out through extra to make it so the filter functions can be used without explicit I/O.

isullivan commented 2 years ago

I think it is worth having visibility_count as its own function, but agree that it is problematic that it duplicates code from visibility_grid. If we could write it so that visibility_count was actually used by visibility_grid (and probably visibility_degrid as well) to do the preparatory calculations, that would probably be best. Otherwise, perhaps just print a warning if it is used to let the user know that the results might not be identical to those from visibility_grid. If the warning is prominent enough, hopefully someone will notice if it then turns out that this function is still being used.

SkyWa7ch3r commented 2 years ago

This particular issue came up as during translating filter_uv_uniform, obs, psf and params are loaded in by visibility_count even though, the function which calls filter_uv_uniform, has originally been called by visibility_grid.pro.

Following the function calls we get visibility_grid --> grid_beam_per_baseline --> dirty_image_generate --> filter_uv_uniform which could pass through the obs, psf, params from visibility_grid and the weights from , rather than loading them in.

Given that filter_uv_uniform can already take these in as parameters, its likely better for performance and memory use to not recalculate or to reload in the data if its already there in memory.

With that said, if visibility_count is used extensively throughout FHD, then altering visibility_count as required to get feature parity with visibility_grid, and then using visibility_count directly in visibility_grid is also best to make the code as modular as possible.

Even then though, it would still be better to pass them through as at least in Python it will just create another reference to obs, psf, params and weights, rather than having to recalculate them and use extra memory (even if it may be minimal).