tart-telescope / tart2ms

Convert TART data to measurement set format
GNU General Public License v3.0
1 stars 1 forks source link

Code and CLI cleanup #23

Closed tmolteno closed 1 year ago

tmolteno commented 1 year ago

There are a few suggestions for changes after merging @bennahugo's new phase centre features.

  1. Remove the uvw_generator option to ms_create: This should always use casacore. The testbench checks that they agree for a single snapshot.
  2. Rename command line option override-telescope-name to --telescope-name (with the default of TART and a note about using KAT-7 if CASA compatibility is needed)
  3. Update description of phase-center-policy. I think the words 'dump' and 'observation' are confusing. see below
  4. Add some nice examples to the readme, and to the notebooks.

Phase centre options

These seem to be the alternatives. I'd like to make a clear set of command line options that describe this

  1. The phase-center is a fixed point in the celestial coordinate system. We currently use the celestial coordinates of the zenith at a specific time (midpoint). How about --phase-center='celestial-tracking'. we should clearly let the user know what the new phase center is.
  2. The phase-center is fixed in the local geodetic coordinates (no phase rotations are done). How about something --phase-center=no--tracking

Comments welcome. We want this to be understandable by someone new to radio telescopes.

bennahugo commented 1 year ago

I will have a look but the current midpoint snapping is a kludge.... it will be wrong for anything but a short observation. I think this actually deserves a fat warning. I will deal with it in the rephasor operator. We can do 'instantaneous-zenith','rephase-obs-midpoint','norephase-obs-midpoint'. The last should sound alarm bells -- I'm actually of the opinion it should not exist...

bennahugo commented 1 year ago

I'm not a fan of mentioning KAT7 -- we should aim to get #11 solved. Lets get in touch with Bryan Butler about this. I will write him to ask

tmolteno commented 1 year ago

Yes. No need to mention KAT7.

tmolteno commented 1 year ago

After a bit of use, I wonder whether we could modify the CLI slightly. Suggest we consider

    -- rephase [SCP, NCP, obs_midpoint, JXXXX.XXXX] 

Then the default is to do no rephasing. This would correspond to the instantaneous zenith being used as the phase centre of each field. THis makes the command line almost readable.

tart2ms --rephase SCP --hdf my.hdf
bennahugo commented 1 year ago

We could. Do you want to use 12th ball J convention for the last.

Note that this removes the option not to do any phasing but set the observation field centre to the observation centre. Maybe you still want an option for that, perhaps obs_midpoint_norephase or something similar.

On Tue, 25 Oct 2022, 12:32 Tim Molteno, @.***> wrote:

After a bit of use, I wonder whether we could modify the CLI slightly. Suggest we consider

-- rephase [SCP, NCP, obs_midpoint, JXXXX.XXXX]

Then the default is to do no rephasing. This would correspond to the instantaneous zenith being used as the phase centre of each field.

— Reply to this email directly, view it on GitHub https://github.com/tart-telescope/tart2ms/issues/23#issuecomment-1290333527, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4RE6WEBU3SZE7SKFWBVE3WE6ZNLANCNFSM6AAAAAARJPPXHM . You are receiving this because you were assigned.Message ID: @.***>

bennahugo commented 1 year ago

Also I actually prefer to keep the telescope uvw as a way to check things at API level (not a user option). It has come in quite handy yet again to explain why things are flipped in image space. See the ticket on disko. So I think we should scratch that request to remove it?

On Tue, 25 Oct 2022, 20:50 Benna Hugo, @.***> wrote:

We could. Do you want to use 12th ball J convention for the last.

Note that this removes the option not to do any phasing but set the observation field centre to the observation centre. Maybe you still want an option for that, perhaps obs_midpoint_norephase or something similar.

On Tue, 25 Oct 2022, 12:32 Tim Molteno, @.***> wrote:

After a bit of use, I wonder whether we could modify the CLI slightly. Suggest we consider

-- rephase [SCP, NCP, obs_midpoint, JXXXX.XXXX]

Then the default is to do no rephasing. This would correspond to the instantaneous zenith being used as the phase centre of each field.

— Reply to this email directly, view it on GitHub https://github.com/tart-telescope/tart2ms/issues/23#issuecomment-1290333527, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4RE6WEBU3SZE7SKFWBVE3WE6ZNLANCNFSM6AAAAAARJPPXHM . You are receiving this because you were assigned.Message ID: @.***>

tmolteno commented 1 year ago

@bennahugo Yes let's use the following names for the field:

tmolteno commented 1 year ago

Here is the proposed new CLI (going in on the new branch)...

```
New CLI:    --rephase <target>  Perform rephasing of _all_ input visibility snapshots (single integration visibility fields) to the specified phase center.
                            This does not change the number of fields that will be output. There will be one field with name J_XXXXXX per visibility integration.

            --single-field  Merge all the fields into a single field with a common phase center. This means that imaging software will image just this one field by default
                            Useful for deep imaging.

Equivalents for the old --phase_center_policy CLI

                        instantaneous-zenith    : Default options. Each snapshot is a separate field
                        rephase-obs-midpoint    : --rephase obs-midpoint --single-field
                        no-rephase-obs-midpoint : --rephase obs-midpoint
                        rephase-SCP             : --rephase SCP --single-field
                        rephase-NCP             : --rephase NCP --single-field
```
bennahugo commented 1 year ago

Ok I think I'm happy with this This option is a tad bit weirdly worded though no-rephase-obs-midpoint : --rephase obs-midpoint. There is no phasing involved. perhaps it should be --single-field without the rephasing option because there is no phasing (fringe stopping) involved.

So perhaps: rephase-obs-midpoint : --rephase obs-midpoint --single-field no-rephase-obs-midpoint : --single-field rephase-SCP : --rephase SCP --single-field rephase-NCP : --rephase NCP --single-field

tmolteno commented 1 year ago

Totally agree with your point that its a bit weird. I just checked the code and I had put it in as you suggested!!