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

ddid fixes #79

Merged
merged 3 commits into from
Feb 3, 2021
Merged

ddid fixes #79

merged 3 commits into from
Feb 3, 2021

Conversation

smasoka
Copy link
Contributor

@smasoka smasoka commented Jan 25, 2021

Fixes the indexing discovered by #77

@ratt-priv-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@smasoka
Copy link
Contributor Author

smasoka commented Jan 25, 2021

added the computing meta fix you did @sjperkins

The real test with MS from @IanHeywood 15A-310.sb30704857.eb31073319.57261.07368590278_spw24.ms is still running

@bennahugo
Copy link
Collaborator

ok to test

Copy link
Collaborator

@bennahugo bennahugo left a comment

Choose a reason for hiding this comment

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

Looks good to me thanks

@bennahugo
Copy link
Collaborator

Feel free to post before and after plots of the VLA data here for discussion

@sjperkins
Copy link
Member

Feel free to post before and after plots of the VLA data here for discussion

I'm not sure if this is possible! #77 is caused by is caused by two wrongs making a right for MeerKAT data, but failing on other multi-SPW datasets.

Copy link
Member

@sjperkins sjperkins left a comment

Choose a reason for hiding this comment

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

Looks good to me, just need to remove commented out code.

tricolour/apps/tricolour/app.py Outdated Show resolved Hide resolved
tricolour/apps/tricolour/app.py Outdated Show resolved Hide resolved
@sjperkins
Copy link
Member

Feel free to post before and after plots of the VLA data here for discussion

I'm not sure if this is possible! #77 is caused by is caused by two wrongs making a right for MeerKAT data, but failing on other multi-SPW datasets.

Thankfully @IanHeywood was intrepid enough to try it on something with interesting SPW's.

@bennahugo
Copy link
Collaborator

Feel free to post before and after plots of the VLA data here for discussion

I'm not sure if this is possible! #77 is caused by is caused by two wrongs making a right for MeerKAT data, but failing on other multi-SPW datasets.

Thankfully @IanHeywood was intrepid enough to try it on something with interesting SPW's.

Not sure I follow. One of the datasets has all the original DDIDs but no rows in the MS aside from one. The other has only one SPW and associated rows. Either way it should now be able to flag multiple SPW. Could we plot data without flags (add the flags in casa plotms, should display in red) and with flags, just to be sure it does something logical here?

@sjperkins
Copy link
Member

Feel free to post before and after plots of the VLA data here for discussion

I'm not sure if this is possible! #77 is caused by is caused by two wrongs making a right for MeerKAT data, but failing on other multi-SPW datasets.

Thankfully @IanHeywood was intrepid enough to try it on something with interesting SPW's.

Not sure I follow. One of the datasets has all the original DDIDs but no rows in the MS aside from one. The other has only one SPW and associated rows. Either way it should now be able to flag multiple SPW. Could we plot data without flags (add the flags in casa plotms, should display in red) and with flags, just to be sure it does something logical here?

Ah I see. To reiterate: MeerKAT data only has a single SPW and the code prior to this code only worked for a single SPW. @IanHeywood tried it on an MS with multiple SPW's, exposing the logic error.

Thus, I didn't think it would be possible to get "before the bug" plots. However, I understand that you're asking for plots with and without flagging for the VLA MS on this branch? Got a handy shadems command for @smasoka to try?

@bennahugo
Copy link
Collaborator

bennahugo commented Jan 25, 2021 via email

@bennahugo
Copy link
Collaborator

bennahugo commented Jan 25, 2021 via email

@sjperkins
Copy link
Member

I guess there's also the question of whether our default MeerKAT channel masks apply to other instruments...

I would guess not?

@bennahugo
Copy link
Collaborator

bennahugo commented Jan 25, 2021 via email

@sjperkins

This comment has been minimized.

@smasoka

This comment has been minimized.

@sjperkins

This comment has been minimized.

@bennahugo
Copy link
Collaborator

A followup on this. Where is the VLA dataset so that I can test whether this branch does the right thing in the case of multi-spw?

@bennahugo
Copy link
Collaborator

I do have VLA archival data on my NAS that I can use to test, but it is not P-band as with @IanHeywood 's case.

@bennahugo
Copy link
Collaborator

I'm going to deploy my spidery powers and merge this. I actually need the dask bug fix in order to do some urgent commissioning. I will get to looking at VLA data later - that is a lower priority item

@bennahugo bennahugo merged commit 536ee52 into ratt-ru:master Feb 3, 2021
@bennahugo
Copy link
Collaborator

Thanks @smasoka

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

Successfully merging this pull request may close these issues.

4 participants