EoRImaging / FHD

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

HERA/PAPER related fixes and hacks #48

Closed nicholebarry closed 8 years ago

nicholebarry commented 8 years ago

This issue will document some of the hacks and issues that the collective group at the Cape Cod busy week has found and made. Please contribute with an approximate line number, the associated error, and the solution.

We'll gracefully put them into FHD in due course, but documenting them is half the battle. Also, documenting some of the HERA/PAPER specific keywords would be great (number of freq averaged channels, etc.)

aelanman commented 8 years ago
nicholebarry commented 8 years ago

The instrument_config inside of the simulation subfolder does not have a hera equivalent, but it seems to not matter. Is this folder depreciated? We should mark it as such if so.

jkerrigan commented 8 years ago

Issue with vis_calibrate.pro (Line 190) being passed an empty Null pointer that cannot be dereferenced. Using FHD on a paper64 observation with no autocorrelations in the uvfits, however the keyword in vis_calibrate.pro, vis_auto_model are set to 1 which leads to vis_cal_auto.pro being called when it should not be, because there are no autocorrelations. Workaround is to set vis_auto_model = 0, or add in an additional keyword that checks to see if autocorrelations exist in the first place.

isullivan commented 8 years ago

Issue with vis_calibrate.pro (Line 190) being passed an empty Null pointer that cannot be dereferenced.Using FHD on a paper64 observation with no autocorrelations in the uvfits, however the keyword in vis_calibrate.pro, vis_auto_model are set to 1 which leads to vis_cal_auto.pro being called when it should not be, because there are no autocorrelations.

Fixed

isullivan commented 8 years ago

Line 67 of fhd_core/beam_setup/fhd_struct_init_antenna.pro. This line calculates psf_dim by doubling the antenna size and converting it from meters to uv bin size. This is divided by the cosine of the zenith angle, shortening psf_dim the closer the source is to the zenith. Since HERA always points to zenith, this shouldn't matter, but we noticed it and Ian recommended we cut the cosine term.

Fixed

isullivan commented 8 years ago

FHD calculates beam models for each course frequency channel. To my understanding, HERA doesn't have course channels, so simulating HERA accurately meant setting nfreq_avg to 1 (so there's one "course" channel per frequency channel). For a large number of frequency channels, this subsequently eats up memory on the beam_setup step (over 100G for 200 channels). Unsure of how to address this.

There's nothing magical about the coarse frequency channels of the MWA in FHD, so you can set nfreq_avg to whatever you want.

isullivan commented 8 years ago

The instrument_config inside of the simulation subfolder does not have a hera equivalent, but it seems to not matter. Is this folder depreciated? We should mark it as such if so.

The instrument_config in simulation IS used, so it's a potential bug that it doesn't crash. It looks like the 'instrument' keyword is not passed in correctly, so it's been using the mwa config file. I will fix the keyword passing bug, but someone else will need to fill in the values for hera_simulation_instr_config.pro.

nicholebarry commented 8 years ago

Okay, I'll see if I can throw one together real quick.

aelanman commented 8 years ago

What did it mean? If this behavior was a source of confusion, I can add messages that state when one of those keywords is overridden by another

Whoops, left that comment hanging before.... It's fixed now. Comments would be helpful here, especially if they could clarify the relationships among the three variables -- dimension, psf_dim, and kbinsize.

There's nothing magical about the coarse frequency channels of the MWA in FHD, so you can set nfreq_avg to whatever you want.

Bryna pointed this out to me earlier. I'll have to think of how best to set n_freq_avg for my HERA simulations. The memory usage when n_freq_avg = 1 seemed kindof extreme.

isullivan commented 8 years ago

Whoops, left that comment hanging before.... It's fixed now. Comments would be helpful here, especially if they could clarify the relationships among the three variables -- dimension, psf_dim, and kbinsize.

Fixed. I've cleaned up the code at that point to make the logic more clear, and added warning messages if any user-supplied values are over-written.

nicholebarry commented 8 years ago

The instrument_config inside of the simulation subfolder does not have a hera equivalent, but it seems to not matter. Is this folder depreciated? We should mark it as such if so.

Added the file.

isullivan commented 8 years ago

I believe all of the issues mentioned have been fixed, except for the beam image output with fhd_quickview. I can't replicate the observed behavior, so I'm not sure how to fix it. If anyone else (nichole?) sees similar behavior, please let me know.

If there are no other immediate bugs, I'd like to close this issue.

nicholebarry commented 8 years ago

Both Josh and Adam saw the issue with the HERA beam. I can reproduce it for you if you want and if they want to share there input uvfits. It was specifically with cgloadct and the invert keyword. I'll investigate it more if you want, but it's definitely not critical (just a little unnerving when looking at the HERA beam for the first time!).

Go ahead and close. Thanks Ian