Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modified gleam #117

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

samirchoudhuri
Copy link

A method added in the utils.py to generate a modified GLEAM catalogue which fills the blank regions.

@bhazelton bhazelton self-requested a review January 26, 2021 16:15
@bhazelton bhazelton added the enhancement New feature or request label Jan 26, 2021
@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #117 (035fccd) into main (22fab17) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #117      +/-   ##
==========================================
- Coverage   99.71%   99.51%   -0.20%     
==========================================
  Files           5        6       +1     
  Lines        1399     1448      +49     
==========================================
+ Hits         1395     1441      +46     
- Misses          4        7       +3     
Impacted Files Coverage Δ
pyradiosky/utils.py 100.00% <100.00%> (ø)
pyradiosky/__init__.py 62.50% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22fab17...035fccd. Read the comment docs.

Copy link
Member

@bhazelton bhazelton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together @samirchoudhuri!

I have a few comments about the code, particularly around better integrating it with the SkyModel object. Also, we'll need to get all the new lines covered with tests.



def modified_gleam(
gleam_filename="gleamegc.dat",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do you get this file from? We have a method in this utils module to download the GLEAM catalog as a VOT table from Vizier. But it isn't just a text file like it looks like you're expecting this one to be.

raise ValueError("File not found")

# read array of RA (deg), Dec (deg), Flux at 100 MHz (Jy), sp. index
aa = np.genfromtxt(gleam_filename, usecols=usecols)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a good reason not to use the VOT table and the read_gleam_catalog method on SkyModel to get the values you need here?

aa = np.genfromtxt(gleam_filename, usecols=usecols)

# use sources with finite flux and sp. index values
cat_gleam = aa[(aa[:, 2] >= 0.0) & np.isfinite(aa[:, 2]) & np.isfinite(aa[:, 3])]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So leaving out the sources without a spectral index concerns me. This is ¼-⅓ of the catalog and it is not limited to dim sources. Bright sources that have peaked spectra or broken power law shapes also end up without a spectral index. The read_gleam_catalog method on SkyModel can read in all the subband fluxes and you can interpolate between them if needed.

Comment on lines +375 to +385
[201.3667, -43.0192, 1937.472, -0.50], # Centaurus A
[139.5250, -12.0956, 544.686, -0.96], # Hydra A
[79.9583, -45.7789, 774.612, -0.99], # Pictor A
[252.7833, 4.9925, 791.486, -1.07], # Hercules A
[187.7042, 12.3911, 1562.747, -0.86], # Virgo A
[83.6333, 22.0144, 1560.743, -0.22], # Crab
[299.8667, 40.7339, 13599.676, -0.78], # Cygnus A
[350.8667, 58.8117, 15811.361, -0.41], # Cassiopeia A
[50.6708, -37.2083, 1256.016, -0.81],
]
) # Fornax A
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did these come from? I'd really like to document the source of these in the code. We actually have some other students working on getting better (multi-component) models for these sources into SkyModel objects, so I've been thinking about making a function to add them to GLEAM. We don't have those ready yet, so we can start with these and then replace them with better ones, but I'm insisting that we have a documentation trail for our sky models.

Comment on lines +388 to +390
# save in a file
if modified_gleam_filename != "":
np.savetxt(modified_gleam_filename, cat_gleam)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel pretty strongly that we want to actually use the SkyModel object here. It is similar to UVData in that it requires that all the necessary metadata are set and then it has write methods (I'd prefer to use the write_skyh5 method) to write to standardized file formats that can be read back into a SkyModel object in the future.

if modified_gleam_filename != "":
np.savetxt(modified_gleam_filename, cat_gleam)

return cat_gleam
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this should return a SkyModel object, not a numpy array.

Comment on lines +294 to +295
Write modified gleam catalogue to fill the blank regions, and also to add the peeled sources.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fill out this docstring a little more to describe the algorithm in a bit more detail? You can even put in a reference to your paper if you like. But other people are much more likely to use it if they understand what it's doing and what problem it's trying to solve.

@mkolopanis
Copy link
Member

Hey @samirchoudhuri Bryna is offline for now, but I'm around to help you get this over the finish line once you get back around to it. Great place to start by addressing all of her last comment :D

@samirchoudhuri
Copy link
Author

Thanks, @mkolopanis. Sure, I will get back to you soon.

@bhazelton
Copy link
Member

@samirchoudhuri I'm back and happy to help push this forward if needed. It looks like it needs to be rebased again which I can also help with.

@samirchoudhuri
Copy link
Author

Thanks @bhazelton. I was working on this PR, but got stuck in converting to SkyModel object. I will resume and update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants