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

Fix array_finalize for beams #75

Merged
merged 16 commits into from Jan 17, 2019

Conversation

e-koch
Copy link
Collaborator

@e-koch e-koch commented Jan 16, 2019

minajor -> minor in array_finalize for Beams.

I found this trying to do arithmetic with a Beams object, which also doesn't work correctly. But I'm not sure if we need it to?

@e-koch e-koch requested a review from keflavich January 16, 2019 22:55
@keflavich
Copy link
Contributor

Oops, that's a bug.

Why not have a majors * minors * 2 * pi / fwhmfactor test instead of assuming symmetric beams?

@e-koch
Copy link
Collaborator Author

e-koch commented Jan 16, 2019

I didn't get that far in the test. Here's an updated version, but it's not working. The original self object seems to lose its attributes? I don't understand where array_finalize is getting called when doing an operation and my assumption that the original and new sr are both accessible is probably wrong

@keflavich
Copy link
Contributor

Right. I don't think arithmetic operations should work on Beams or Beams, it doesn't really make a ton of sense.

@e-koch
Copy link
Collaborator Author

e-koch commented Jan 16, 2019

Multiplication/division could be made to work if applied on the areas. But I can't think of a case where you would want to that.

We should probably raise some sort of InvalidBeamOperationError. Right now it happily returns wrong answers without error.

Addition/subtraction can be treated as convolution/deconvolution like it does in Beam

@keflavich
Copy link
Contributor

on areas: why not just do Beams.sr * 5 instead of (Beams*5).sr?

And, yes, agreed, raising descriptive errors is a good idea

@e-koch
Copy link
Collaborator Author

e-koch commented Jan 17, 2019

I've defined a number of operations for Beams to match (mostly) the operations in Beam.

One weird thing in Beam: convolution is multiplication, but deconvolution was subtraction? I think making it division makes more sense as the opposite operation

@keflavich
Copy link
Contributor

agreed, division and multiplication make more sense as the de/convolve operations

@e-koch
Copy link
Collaborator Author

e-koch commented Jan 17, 2019

I've kept the __sub__ operator in Beam in case it's a breaking change. Otherwise, mult/div are used.

I'm happy to merge this. Let me know if you have any more comments.

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

minor: are we deprecating subtraction-as-deconvolution?

radio_beam/multiple_beams.py Outdated Show resolved Hide resolved
radio_beam/tests/test_beams.py Outdated Show resolved Hide resolved
@e-koch
Copy link
Collaborator Author

e-koch commented Jan 17, 2019

@keflavich - I think we should deprecate subtraction-as-deconvolution. I'll add a warning

@e-koch e-koch merged commit 69eac92 into radio-astro-tools:master Jan 17, 2019
@e-koch e-koch deleted the fix_beams_arithmetic branch January 17, 2019 18:23
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.

None yet

2 participants