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

Mauchian #1215

Merged
merged 6 commits into from
Jul 31, 2020
Merged

Mauchian #1215

merged 6 commits into from
Jul 31, 2020

Conversation

dane-kleiner
Copy link
Collaborator

Implements the option of using the Mauch et al. (2020) primary beam correction. Addresses the primary beam correction in #1180, although not for the mosaicking.

@dane-kleiner dane-kleiner requested a review from paoloserra July 27, 2020 09:26
@ratt-priv-ci
Copy link
Collaborator

Can one of the admins verify this patch?

paoloserra
paoloserra previously approved these changes Jul 27, 2020
@dane-kleiner
Copy link
Collaborator Author

Is this hanging? @paoloserra

@paoloserra
Copy link
Collaborator

Shouldn't be, it just takes a little time.

@paoloserra
Copy link
Collaborator

ok, this is now too long. @gigjozsa @KshitijT could you please check what's up?

@gigjozsa
Copy link
Collaborator

Retest this please

@paoloserra
Copy link
Collaborator

@dane-kleiner I've hijacked your PR to make @o-smirnov happy and close #1214 , hope you don't mind!

@gigjozsa
Copy link
Collaborator

Doesn't work...

@gigjozsa
Copy link
Collaborator

I don't have any idea. Jenkins is not activated.

@paoloserra
Copy link
Collaborator

ok to test

@paoloserra paoloserra merged commit e906138 into master Jul 31, 2020
@paoloserra paoloserra deleted the mauchian branch July 31, 2020 05:58
})
if config[key]['cleanmask_localrms'][num-1 if len(config[key]['cleanmask_localrms']) >= num else -1]:
Copy link
Member

Choose a reason for hiding this comment

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

This isjust cosmetics of course, but in the interest of readability (who will think of the future generations!), I would prefer to phrase that as

local_rms = config[key]['cleanmask_localrms'][min(num-1, len(config[key]['cleanmask_localrms'])-1)]
if local_rms:
    image_opts.update({
        "local-rms": local_rms, 
        "local-rms-window": config[key]['cleanmask_localrms_window'][min(num-1, len(config[key]['cleanmask_localrms'])-1)]
    })

...since that if-else construct is a big eyeful to take in, and recurs a lot.

Even better, define a helper function in there:

def n_1_or_last(seq, num):
    return seq[min(num-1, len(seq)-1)]

...since I presume this treatment of lists happens all over the place?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah that would be neater -- we can sneak that into the next PR

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.

selfcal worker has hardwired local-rms=True and auto-mask settings
5 participants