Gbo #15
Merged
Gbo #15
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrs65
requested changes
Mar 12, 2021
There was a problem hiding this comment.
Hi @leungcalvin
This is looking good, and I don't think it needs a huge amount more work before it's ready to merge. Here are some initial comments:
- Please rebase this into a set of logical commits with messages following our dev guidelines (https://github.com/chime-experiment/Pipeline/blob/master/CONTRIBUTING.md#rules). I'll leave you to figure out exactly what to do, but you need at least these changes in separate commits: changing the default location for CHIME; adding support for changing the observatory to the delay/fringestop routines; and adding support for TONE.
- I'm a bit averse to having a dump of all the antennas in directly, just because it dumps in a lot of redundant info. Can you wrap it up into something which programmatically generates them?
- There's a lot of commented out lines (e.g. old params, print statements, etc). Please tidy those up.
- Please fix up the merge conflicts, I think the CI won't run until you have.
Hi Richard, I've addressed these comments in a series of logically-chunked commits. |
jrs65
requested changes
Apr 26, 2021
ch_util/tools.py
Outdated
Comment on lines
2227
to
2239
| # print("ha", ha) | ||
|
|
||
| latitude = np.radians(ephemeris.CHIMELATITUDE) | ||
| latitude = np.radians(obs.latitude) | ||
|
|
||
| # Get feed positions / c | ||
| feedpos = get_feed_positions(feeds, get_zpos=wterm) / scipy.constants.c | ||
| feed_delays = np.array([f.delay for f in feeds]) | ||
| # print("feed_delays", feed_delays) | ||
| # print("feed_pos", *feedpos.T[..., np.newaxis]) | ||
|
|
||
| # Calculate the geometric delay between the feed and the reference position | ||
| delay_ref = -projected_distance(ha, latitude, src_dec, *feedpos.T[..., np.newaxis]) | ||
| # print("delay_ref", delay_ref) |
There was a problem hiding this comment.
Please remove all of these commented out print lines.
|
Oh also @leungcalvin when you're done, please rebase this such that only the actual relevant commits are left. I think that should be the three |
added 4 commits
April 26, 2021 12:51
jrs65
previously approved these changes
Apr 26, 2021
jrs65
approved these changes
Apr 28, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactored ephemeris, added best known CHIME phase center position, and included some modifications for GBO/TONE outrigger. Currently the TONEAntenna objects are hard coded. The fix is entangled with the long term solution to
chimedb_configandget_correlator_inputs, but if we're hard coding the array coordinates I see no problem with hard coding the small number of manageable TONEAntennas while we figure out that problem.