feat: ability to do consistency checks on beam lists

[steven-murray]

Description

This adds the check_consistency method on the BeamList class, which can be manually called to check that all of the consistuent beams have some consistent properties. It also adds a test for both passing and failing cases.

Motivation and Context

In simulation, often you're assuming that all your beams come from the same telescope, and have some set of similar properties (eg, all are efield/power and all have the same set of polarizations/feeds etc). This is not always the case, or at least, more general simulators will allow different beams to have different properties and have ways of simulating each one independently. However, many simulators will want some way to check consistency before going off and running. This adds a simple method that can be called in such a case.

I am not the expert on what people may or may not want to check for. I added a keyword check_pols to check for polarization-related stuff (which is almost everything) because I don't know if all simulators will want to check for that. There are other UVBeam parameters that I ignored because I don't know enough about them, but perhaps should be added to the checks (eg. Ncomponents_vec and frequency-related parameters).

I also opted to raise an error if one of the beams doesn't have a certain parameter but the rest do, as well as if they both have the parameter but they're different. I'm not sure this is always the right thing to do. For instance, AnalyticBeam objects don't define Nfeeds even though they're efield beams by default. Not sure how that works.

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] Reference simulation update or replacement
• [ ] Documentation change (documentation changes only)
• [ ] Version change
• [ ] Build or continuous integration change

Checklist:

For all pull requests:

• [x] I have read the contribution guide.
• [x] My code follows the code style of this project.

New feature checklist:

• [x] I have added or updated the docstrings associated with my feature using the numpy docstring format.
• [ ] I have updated the documentation to highlight my new feature (if appropriate).
• [x] I have added tests to cover my new feature.
• [x] All new and existing tests pass.
• [ ] I have checked that I reproduce the reference simulations or if there are differences they are explained below (if appropriate). If there are changes that are correct, I will update the reference simulation files after this PR is merged.
• [x] I have checked (e.g., using the benchmarking tools) that this change does not significantly increase typical runtimes. If it does, I have included a justification in the comments on this PR.
• [x] I have updated the CHANGELOG.

[codecov[bot]]

Codecov Report

Merging #360 (21f1b75) into main (bcaae6b) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #360      +/-   ##
==========================================
+ Coverage   99.15%   99.17%   +0.01%     
==========================================
  Files          13       13              
  Lines        2009     2054      +45     
==========================================
+ Hits         1992     2037      +45     
  Misses         17       17              

Impacted Files	Coverage Δ
pyuvsim/telescope.py	100.00% <100.00%> (ø)
pyuvsim/uvsim.py	98.56% <100.00%> (+0.01%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bcaae6b...21f1b75. Read the comment docs.

[bhazelton]

This looks good, but I actually have some work I started on another branch that addresses the issue of the x_orientation and I'd like to figure out how that fits in with this.

In my branch I added a check for the x_orientation of the beams in the BeamList and errored if they conflicted and warned if there was no x_orientation set for any of the beams that have that attribute. Then I used the x_orientation of the beams to set the x_orientation of the final UVData object. @aelanman I ran into some trouble with testing my code because it seems that the x_orientation is not preserved through the conversion to a string representation and back. I think this is because it's not getting captured in the uvb_params dict, but I can't tell where the definition is for what attributes of a UVBeam object are captured in that dict. I'd like to update it to capture x_orientation but I don't know how. Can you help me find the relevant bit of code there? Or do I need to update that in the file for it to work properly?

[aelanman]

@bhazelton I think what you're looking for is the _float_params dictionary defined at the start of the BeamList class.

[bhazelton]

@aelanman I don't think so, those are only used for analytic beams, I need something for UVBeams.

[bhazelton]

@steven-murray Do you want to rebase this PR and adjust it light of the changes in #363?

[steven-murray]

@bhazelton yes, thanks for reminding me.

So, we have a couple of options here. I have written a method check_consistency, which is meant to be manually called. It checks consistency of beam_type, x_orientation, Nfeeds, polarization_array and feed_array. So, one idea for incorporating your changes (which effectively do the x_orientation check by default) would be to have an option check: bool = True in the constructor that calls check_consistency. This way by default all these keywords, including x_orientation, would be checked.

Otherwise, I could take x_orientation out of check_consistency and let it just be checked in the constructor apart from the rest.

[bhazelton]

I'm fine with doing all the checks by default. But I'm not sure we want to allow users to turn some of them off.

I think something went wrong with the rebase, there are a lot of changes in this PR that should not be here. Did you maybe pull after the rebase?

[steven-murray]

Yeah, I think I pulled after rebase. Sorry, I'm not fluent in rebasing (I always do merging for my own repos). How should I fix this?

[bhazelton]

Undo the last commit (git checkout HEAD~1) then do a force push (git push -F). You can check your work before the force push if you want to. I use a GUI for this, to make sure that the branch has the right commits and changes with no merge commits.

[steven-murray]

@bhazelton another question: running check_consistency only works for object mode (not str mode). Is it worth converting to object mode, doing the check, and converting back, if it starts in string mode? the other option would be to put the check_consistency() call into the set_obj_mode call, so whenever you convert to object mode it does the check?

[bhazelton]

So looking at the things you're checking (easier now with the rebase fixed) I think all of this should happen no matter what (users should not be able to turn it off) and we should convert to an object type to do these tests (which I did in my implementation of the x_orientation check). On the beam type, pyuvsim requires an efield beam, if the beam is a power beam it should just error.

I'm happy for you to either put all these checks into the same place that I put my x_orientation check or to pull that one out to go with these (I don't think we want them split up) but I think these should all be mandatory.

[steven-murray]

@bhazelton I'm not as convinced that we should make them mandatory. My reasoning is that pyuvsim is supposed to provide an interface for performing simulations (as well as an actual simulation engine). For example, hera_sim uses the BeamList as an element in its interface, which needs to be able to apply to many different simulators. In this case, not all simulators expect efield beams (whether they should or not is another question). Thus, we should not, in my opinion, have a mandatory check for the beams being efield beams. We should just provide easy tools to do that check in case a particular simulator requires it.

[bhazelton]

Ok, let me rephrase: there should be a mandatory call checking these things in the part of pyuvsim that actually does the simulations.

[steven-murray]

@bhazelton thanks for the review: I'll incorporate those ideas. Unfortunately tests are failing locally atm, and I haven't had time to sort through it (and won't till Nov 15).

[bhazelton]

ping on this @steven-murray

[steven-murray]

@bhazelton I've fixed up the comments you had. However, I'm getting quite a few test failures, due to beamlists in the tests that are not consistent (previously existing tests that were previously passing). I could turn off the checks in these tests, or I could update the beamlists, or change the code so that these are not considered "inconsistent". I don't think I'm familiar enough with the code to know which is best to do...

[steven-murray]

If it helps, the majority of test failures are for BeamConsistencyError: beam 2 does not have x_orientation but beam 1 does

[steven-murray]

@bhazelton this is ready for another review. The external herasim tests are failing because we explicitly create non-consistent beamlists to test there (we catch them in hera_sim, but once this PR is in, we can outsource those checks).

There is one uncovered line in the diff, but I didn't add that line, so it must have been uncovered previously, but I just moved the line or something. Up to you if you want to get that covered in this PR.

[steven-murray]

@bhazelton I think I have addressed all the comments, though it might need to be rebased on main.

[bhazelton]

@steven-murray this looks good, but the tests are failing. Do you want me to do the rebase? I'm happy to.

[steven-murray]

@bhazelton I rebased, and fixed the failing test that I botched. However, there are other failing tests that arise due to new warnings being emitted by distutils (nothing to do with this PR, but obviously an updated package?) Do you have ideas there? Feel free to go ahead and fix directly if you know what's happening.

[bhazelton]

The PR I just merged fixes these distutils deprecations warnings that were breaking the tests. So I can rebase this if that's ok with you and I think the tests will be fixed.

[steven-murray]

@bhazelton Go ahead! Thank you.

[bhazelton]

@steven-murray I just realized that you made BeamList._obj_to_str a class method but you left BeamList._str_to_obj as a regular method. Is that what you wanted to do? I don't really see why BeamList._obj_to_str needs to be a class method given the ways it's called, but it feels a weird to have this asymmetry between them.

[steven-murray]

@bhazelton I couldn't get _str_to_obj to be a classmethod because it has a reference to self in it. I did feel icky about the asymmetry, and would feel better if both were class methods. I'm happy to take out the @classmethod from _obj_to_str if you prefer, or just leave the asymmetry.

[bhazelton]

What is the added utility in making _obj_to_str a classmethod?

[steven-murray]

Mostly it's a matter of principal -- I usually try to make things class methods if it's possible to. It means I don't have to instantiate an object to use that functionality, which can come in handy later. For example, maybe one day we want to use this function to convert a beam object to/from obj/str where we've received that object from elsewhere. We could always create an instance of course, but it seems cleaner not to if we don't have to.

[bhazelton]

Interesting. I tend to prefer to move things that do not need to be on an object (because e.g. they don't use the attributes) to functions rather than class methods. In this case, I think I'd prefer to leave them as regular methods. They are currently not part of the public API (they start with an underscore). If they become useful outside the BeamList object, I'd prefer to turn them into public functions instead.

[steven-murray]

I can definitely understand that reasoning. OK, I have removed the classmethod decorator.

[bhazelton]

Darn, it can't be merged because of conflicts. Probably because of the merge commit. I'll fix that now.