feat(tools): Support querying correlator inputs for CHIME/FRB Outriggers

[aaronpearlman]

There was no logic for handling allenby_cylinder when using tools.py --> get_correlator_input(..., correlator="allenby").

This introduced the following errors when using get_correlator_input to query the correlator inputs for Allenby:

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-18-f2cce6172052> in <module>
      1 inputmap = tools.get_correlator_inputs(ephemeris.unix_to_datetime(f['timestamp'][0, 0]['ctime']),
----> 2                                                    correlator=None)

/usr/local/lib/python3.6/site-packages/ch_util/tools.py in get_correlator_inputs(lay_time, correlator, connect)
   1235     inputlist = [
   1236         _get_input_props(layout_graph, *args)
-> 1237         for args in zip(inputs, corrs, rfls, rfi_ants, noise_sources)
   1238     ]
   1239 

/usr/local/lib/python3.6/site-packages/ch_util/tools.py in <listcomp>(.0)
   1235     inputlist = [
   1236         _get_input_props(layout_graph, *args)
-> 1237         for args in zip(inputs, corrs, rfls, rfi_ants, noise_sources)
   1238     ]
   1239 

/usr/local/lib/python3.6/site-packages/ch_util/tools.py in _get_input_props(lay, corr_input, corr, rfl_path, rfi_antenna, noise_source)
    623     }
    624 
--> 625     cyl = pos_dict[rfl.sn]
    626 
    627     # Different conventions for CHIME and Pathfinder

KeyError: 'allenby_cylinder'

This PR implements skeleton classes for the Allenby, GBO, and HCRO telescopes. It also defines the logic for handling this error when the condition on cyl = pos_dict[rfl.sn] is evaluated.

[kiyo-masui]

From Aaron, pasted from Slack:

Summary of the changes:

• Skeleton classes for the Allenby, GBO, and HCRO outriggers are all set up. We can fill in the details in a future PR, as we compile the critical information for the project.
• New logic for handling allenby_cylinder (and eventually gbo_cylinder and hcro_cylinder) is implemented. This eliminates the error Juan encountered, which is described in the PR (see link above).
• The correlator="allenby" prefix is now defined, so you can query the correlator inputs via get_correlator_inputs(..., correlator="allenby").
• If you want to avoid encountering this error, you’ll need to update ch_util once this is merged in.

I also don’t understand the logic behind why the different cylinders are encoded to different integers in pos_dict. I chose not to re-do that logic to avoid the possibility of creating new issues downstream. So, for now, let’s roll with it.

Note: I have FCA000704 connected to BKC0_A00 in layoutdb, which is connected to the allenby reflector. So, you can test if this works right now if you want. But, for others, you won’t be able to query the correlator inputs at Allenby until this is merged and you update ch_util, as I mentioned above.

[kiyo-masui]

Looks like you have a blocking lint fail, which will be easy to fix.

The key thing here is testing. We shouldn't merge until we are sure it works. On slack Aaron made the argument that we should merge before Mandana enters the full LTF connections, but I don't understand that: any test we can perform on one input, we can perform before merging right? So we might as well merge after we test on the populated database, so at least ch_util is well tested? The alternative is to merge something that is only tested on one chain, in which case we might end up with a broken ch_util and a broken database.

If we want to be paranoid, we could populate a testing (sqlite) database instead and test there. I'm not advocating this.

[kiyo-masui]

Another question: do we want to bake the site name "Allenby" into our code base? I think we've been trying to use pco or princeton instead.

[jrs65]

Another question: do we want to bake the site name "Allenby" into our code base? I think we've been trying to use pco or princeton instead.

You're not limited to just one alias.

[aaronpearlman]

@kiyo-masui My argument for merging before entering Mandana's data was because, if she connects many things in layoutdb from the allenby correlator inputs to the reflector, then it will be more work to remove them. It might be better to construct robust tests on a small number of chains if, for example, there is a possibility that we need to rapidly restore the ability to query the correlator inputs at Allenby (before applying this fix) for some hardware tests. This was the situation when we discovered this problem. It will be much easier to sever one or a few connections rather than many. Sorry if that was unclear!

These changes to ch_util are not going to affect the database, so we won't end up with a broken database because of this. I am reasonably confident now that Don's update to peewee fixed our problems, which I view as a separate issue and unrelated. I agree that others should test and make sure this works, but we can always roll back to the previous commit. I am reasonably confident that this works though. I did not just write code and not test :)

"allenby" is already baked in, in some sense, because that's what Mandana named the site and reflector in layoutdb. We can always re-alias things, as @jrs65 mentioned. It seemed to me that the name for Allenby has a reasonably high probability of potentially changing to something other than "allenby", "princeton", or "pco", so I think we should revisit only when the name is finalized.

[jrs65]

I won't comment on how urgent it is to merge this, although I don't have any objections to merging something which may not work in some localised manner (i.e. limited to allenby queries).

Really I think we should refactor this code soon, it's really become unmanageable. Initially I thought you could extend the purpose of the pos_dict table to get rid of the mass of if ... elif statements, i.e. something like:

pos_dict = {
    "W_cylinder": (0, PathfinderAntenna),
    # ...
}

cyl, ant_class = pos_dict[rfl.sn]

return ant_class(
    id=chan_id,
    input_sn=corr_input.sn,
    corr=corr_sn,
    reflector=rfl.sn,
    cyl=cyl,
    pos=pos,
    pol=pdir,
    antenna=ant.sn,
    rf_thru=rft_sn,
    flag=flag,
)

However, there needs to be a bit of a mechanism for figuring out some of the properties for each location (i.e. determining pos).

I think if you want to get this merged ASAP, then maybe it's okay to leave it, but if not then it would be nice to have this refactored to something more manageable.

[jrs65]

Also, a bit of a minor ask, but could you change the commit message to something with shorter lines (https://github.com/chime-experiment/Pipeline/blob/master/CONTRIBUTING.md#-commit-message-guidelines)? A shorter first line and then any details in a subsequent paragraph would be great.

[aaronpearlman]

@jrs65 I updated the commit string, as you requested.

Re refactoring: Are a lot of these telescope properties calculated and queried using other downstream pipelines? If so, that's a lot more testing that we would need to do to make sure our changes won't break other software. But, overall, I agree the logic/code structure here is clumsy.

My understanding was that there was some urgency to fix problems and get a working implementation for Outriggers commissioning. If that's still true, then I'm in favor of merging in a solution now and adding the refactor to our #TODO list with a more relaxed timeline. @kiyo-masui is one who decides the prioritization though, so I'll defer to him.

[jrs65]

Re refactoring: Are a lot of these telescope properties calculated and queried using other downstream pipelines? If so, that's a lot more testing that we would need to do to make sure our changes won't break other software. But, overall, I agree the logic/code structure here is clumsy.

Testing isn't too bad I think (particularly as a one time thing) and is probably something we want to do anyway. Run the old code for correlator="chime", save out the properties of all the CHIME antennas, run the new code and compare.

I agree that it's a bunch more work to do the refactoring cleanly though.

[aaronpearlman]

The code has been linted and now passes all checks. I'll try to remember to do it in the future before pushing code (sorry about that!).

[kiyo-masui]

Alright, if there is low risk of breaking things we need immediately or on the CHIME or baseband analysis side, and if Richard isn't overly concerned, then I'm happy to go along with @aaronpearlman's testing and deployment plan. Aaron, can you coordinate with @mondana?

[kiyo-masui]

Re refactoring: Are a lot of these telescope properties calculated and queried using other downstream pipelines? If so, that's a lot more testing that we would need to do to make sure our changes won't break other software. But, overall, I agree the logic/code structure here is clumsy.

Testing isn't too bad I think (particularly as a one time thing) and is probably something we want to do anyway. Run the old code for correlator="chime", save out the properties of all the CHIME antennas, run the new code and compare.

I agree that it's a bunch more work to do the refactoring cleanly though.

But @aaronpearlman please do do this test before merging.

[aaronpearlman]

I have now compared the output of get_correlator_inputs(..., correlator="chime") with this new version of tools.py and the previous version. They return the same results and same properties for each of the CHIME antennas.

@jrs65 and @kiyo-masui I think this can be safely merged in now, if you both agree.

[jrs65]

Sweet. Thanks @aaronpearlman do you have the code for doing the comparison somewhere. I can definitely see it being useful in the future if we do some more polishing!

[aaronpearlman]

I don't have anything that I think would be useful. The properties of the antennas are listed in the return of get_correlator_inputs, i.e.:

corr_inputs = tools.get_correlator_inputs(datetime.now(), correlator='chime')

So, you can just compare the output of the new code to the old one. It's a simple loop and comparison that anyone should be able to write. If you want access to specific properties of each CHIME antenna, you can get specific properties as follows (for example):

chime_ant_2047 = CHIMEAntenna(id=2047, crate=7, slot=9, sma=3, corr_order=1951, delay=0, input_sn='FCC070903', corr='FCC', reflector='cylinder_D', antenna='ANT0257H', rf_thru='BKR0DC_K03', cyl=5, pol='E', flag=True, pos=[33.05, 38.82, 0.00])

pos_2047 = chime_ant_2047.pos