Conversation
iancze
left a comment
iancze
left a comment
There was a problem hiding this comment.
Looking great so far!
src/mpol/images.py
Outdated
|
|
||
| class PBCorrectedCube(nn.Module): | ||
| r""" | ||
| A ImageCube that has been corrected for the expected primary beam. Currently only implemented for ALMA. |
There was a problem hiding this comment.
As I mentioned in the issue, I think you can be agnostic about which array this is, and split the corrections into two classes, one for a uniform illuminated dish, and the other with a central blockage.
| cell_size=None, | ||
| npix=None, | ||
| coords=None, | ||
| chan_freqs=None, |
There was a problem hiding this comment.
Nitpicky, but I would say freqs to be consistent with other usages.
src/mpol/images.py
Outdated
| self.telescope_to_radius = {"ALMA_12": 6.} # in meters | ||
| self.telescope_to_blocked_radius = {"ALMA_12": 0.375} | ||
|
|
||
| assert (telescope in self.telescope_to_radius) and (telescope in self.telescope_to_blocked_radius) |
There was a problem hiding this comment.
I think separating this into two PB types (uniform and centrally blocked) will simplify the logic here, and make the choice between them more obvious to the user.
src/mpol/images.py
Outdated
| R_obs = telescope_to_blocked_radius[telescope] | ||
|
|
||
| r_coords = np.sqrt(self.packed_x_centers_2D**2 + self.packed_y_centers_2D**2) # units of arcsec | ||
| r_coords_rads = r_coords * np.pi/648000. # radians |
There was a problem hiding this comment.
Where do these constants originate from? If you need speed of light, you can import the variables from constants: https://github.com/MPoL-dev/MPoL/blob/main/src/mpol/constants.py
Eventually we should probably just start depending on astropy, but that's for another day.
src/mpol/images.py
Outdated
|
|
||
| assert (telescope in self.telescope_to_radius) and (telescope in self.telescope_to_blocked_radius) | ||
|
|
||
| self.pb_mask = self.obscured_airy_disk(chan_freqs, telescope) |
There was a problem hiding this comment.
You might want to add a comment here noting that pb_mask is actually in packed cube format. I need to be a bit better about specifying these throughout the ImageCube too.
src/mpol/images.py
Outdated
| def __init__(self, cell_size=None, npix=None, coords=None): | ||
|
|
||
| super().__init__() | ||
| super().__init__(cell_size) |
There was a problem hiding this comment.
Why does cell_size need to be passed here?
src/mpol/precomposed.py
Outdated
| coords=self.coords, nchan=self.nchan, passthrough=True | ||
| ) | ||
|
|
||
| self.pbcube = images.PBCorrectedCube( |
There was a problem hiding this comment.
Rather than modifying SimpleNet, I think it might be better to subclass it as a new SimplePBNet and then make your modifications to that. I think that for learning purposes new users will want to start with literally the simplest thing possible, and the PB correction is a step up from that
|
@kdesoto-psu I started a WIP pull request and added some comments for you here. If you continue to push to your main branch as you would normally do, this should continue to update and we can centralize comments here. |
_setup_coord function replaced with classmethods
Started type hinting Dartboard. The conditional statements can be simplified by rewriting them as guard clauses. Copied attributes from coords can be rewritten as `@property` to save memory
The numpy type hints are a little long, but Python 3.8 doesn't support type aliasing.
"Beautiful is better than ugly."
The init logic can be simplified by treating the if statement as a guard clause
init function has been simplified as well
Trigger GitHub actions on PR from fork
…order pre-release.yml: move linter after mpol install
Modifications to datasets.py
Co-authored-by: Jeff Jennings <jjennings1519@gmail.com>
Co-authored-by: Jeff Jennings <jjennings1519@gmail.com>
Co-authored-by: Jeff Jennings <jjennings1519@gmail.com>
Co-authored-by: Jeff Jennings <jjennings1519@gmail.com>
Co-authored-by: Jeff Jennings <jjennings1519@gmail.com>
Co-authored-by: Jeff Jennings <jjennings1519@gmail.com>
Co-authored-by: Jeff Jennings <jjennings1519@gmail.com>
Co-authored-by: Jeff Jennings <jjennings1519@gmail.com>
Co-authored-by: Jeff Jennings <jjennings1519@gmail.com>
Co-authored-by: Jeff Jennings <jjennings1519@gmail.com>
Co-authored-by: Jeff Jennings <jjennings1519@gmail.com>
… pyro-tutorial
MPoL with Pyro tutorial
…leNet, currently can only handle one channel
…te between channel and spatial frequencies
4 participants
WIP pull request to implement the PrimaryBeam correction for MPoL.