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

Make UVFlag.__add__ iterate over _data_params #844

Merged
merged 7 commits into from
Jun 1, 2020

Conversation

mwilensky768
Copy link

@mwilensky768 mwilensky768 commented May 20, 2020

This PR ensures that all data-like parameters are concatenated appropriately during addition of two UVFlag objects.

Description

The data-like attributes to be concatenated were hardcoded in __add__. Now an iteration over UVFlag._data_params catches all of them, including the optional weights_square_array, if present.

Motivation and Context

This enhancement ensures concatenation of the optional parameter weights_square_array if it exists, and is also a little friendlier to people subclassing UVFlag. See issue #843

Types of changes

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

Checklist:

Bug fix checklist:

  • My fix includes a new test that breaks as a result of the bug (if possible).
  • All new and existing tests pass.
  • I have updated the CHANGELOG.

New feature checklist:

  • I have added or updated the docstrings associated with my feature using the numpy docstring format.
  • I have updated the tutorial to highlight my new feature (if appropriate).
  • I have added tests to cover my new feature.
  • All new and existing tests pass.
  • I have updated the CHANGELOG.

Breaking change checklist:

  • I have updated the docstrings associated with my change using the numpy docstring format.
  • I have updated the tutorial to reflect my changes (if appropriate).
  • My change includes backwards compatibility and deprecation warnings (if possible).
  • I have added tests to cover my changes.
  • All new and existing tests pass.
  • I have updated the CHANGELOG.

Documentation change checklist:

  • Any updated docstrings use the numpy docstring format.
  • If this is a significant change to the readme or other docs, I have checked that they are rendered properly on ReadTheDocs. (you may need help to get this branch to build on RTD, just ask!)

Version change checklist:

  • I have updated the CHANGELOG to put all the unreleased changes under the new version (leaving the unreleased section empty).
  • I have noted any dependency changes since the last version and will update the conda package build accordingly.

Build or continuous integration change checklist:

  • If required or optional dependencies have changed (including version numbers), I have updated the readme to reflect this.
  • If this is a new CI setup, I have added the associated badge to the readme and to references/make_index.py (if appropriate).

@mwilensky768
Copy link
Author

Commit names look silly because I didn't understand how pre-commit works. Also wondering if we should tackle #653 while we're at it.

@mkolopanis
Copy link
Member

The issue you mentioned is hefty but if you feel up to it I would be glad to finally close it.

@mwilensky768
Copy link
Author

mwilensky768 commented May 20, 2020

Well, the branch name is appropriate, but I realize I'd like this change merged in ~soon. Maybe we can do a quick merge when CI passes and work on the next part afterward?

@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

Merging #844 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #844      +/-   ##
==========================================
+ Coverage   99.03%   99.12%   +0.09%     
==========================================
  Files          28       29       +1     
  Lines        9854    10896    +1042     
==========================================
+ Hits         9759    10801    +1042     
  Misses         95       95              
Impacted Files Coverage Δ
pyuvdata/uvflag/uvflag.py 99.37% <100.00%> (+<0.01%) ⬆️
pyuvdata/utils.py 100.00% <0.00%> (ø)
pyuvdata/uvcal/uvcal.py 100.00% <0.00%> (ø)
pyuvdata/uvdata/corr_fits_src/corr_fits.pyx 100.00% <0.00%> (ø)
pyuvdata/utils.pyx 100.00% <0.00%> (ø)

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 0981aac...122b58d. Read the comment docs.

@mkolopanis
Copy link
Member

Just want to pop in here. This will require a rebase now after the windows PR got merged in.

@mwilensky768
Copy link
Author

Rebase complete

Copy link
Member

@mkolopanis mkolopanis left a comment

Choose a reason for hiding this comment

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

Thanks for the work on this @mwilensky768. It makes it much simpler.

I do have one comment about the comment you left concerning the iteration loop and want to discuss how to best handle the cases where optional parameters don't match.

I also have a request that is not strictly related to your PR but during my look at the file i noticed this line should read this.mode, if you wouldn't mind changing it I would appreciate it.

"added to object of mode " + this.type + "."

Comment on lines 1553 to 1554
# Will fail with AttributeError if the objects have different _data_params
# Might want a more straight-forward check ahead of time
Copy link
Member

Choose a reason for hiding this comment

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

This comment makes me a little nervous. I would prefer if the check was added as a part of this PR. We already know they must be the same mode and type which, as long as the user hasn't altered the object's state, forces the required parameters to be the same. So this may not actually be an issue for required parameters.

The other question is what to do if the optional parameters exist on one and not the other?
Erroring seems extreme since it is an optional parameter. We could throw a warning, and then fill in the missing data with zeros? That seems a little weird though too but not sure what the best solution is.

Copy link
Author

@mwilensky768 mwilensky768 May 21, 2020

Choose a reason for hiding this comment

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

I think filling in a weights_square_array with zeros will cause problems down the line in the application I have in mind. I guess if this attribute was vestigial after a mode conversion (metric to flag), and you were adding flag objects, maybe you would not want an error. Could be a warning in one case and an error in the other? Of course, as additional optional parameters are developed, we may have to rethink this line of reasoning.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it definitely gets complicated 🤔 I'm okay with the warning/error differential depending on cases. But we'll still need to eloquently handle when parameters exist on one object and not the other. Maybe it just makes them incompatible in the case where they are not vestigial?

Copy link
Author

@mwilensky768 mwilensky768 May 21, 2020

Choose a reason for hiding this comment

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

I think that's a good start. I don't know what the best check is. Not sure how you feel about try/except. One way would be

for attr in this._data_params:
    try:
        setattr(this, attr, np.concatenate(...))
    except AttributeError:
        vestigial = some_check_for_vestigial (mode=='flag' for now?)
        if vestigial:
            warning
            setattr(this,attr, np.concatenate(...)) (different ... here)
        else:
            raise ValueError("UVFlag objects are not compatible for addition")

Copy link
Member

Choose a reason for hiding this comment

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

I think that could be a good start. I'm not a huge fan of the try/except (despite it being the pythonic way). But could just have an if not hasattr() instead.

Copy link
Author

Choose a reason for hiding this comment

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

Error message could of course be more verbose

Copy link
Author

@mwilensky768 mwilensky768 May 21, 2020

Choose a reason for hiding this comment

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

Oh, yeah if not hasattr is definitely simpler/fewer lines/easier to test. I'll get to work.

@mwilensky768
Copy link
Author

mwilensky768 commented May 21, 2020

I've now added an attribute check that complains if this has a non-null-valued attribute where other has a null-valued one. Additionally, in order that the object passes UVBase.check at the end of __add__, I've made it so that when the weights_square_array is calculated (or if non-null value is read in from a file), it becomes required, and then is forced to be unrequired if mode is switched to flag. If the first part is not done, then an object will never have a weights_square_array due to clear_unused_attributes at the end of to_waterfall. If the second part is not done, one object may have a vestigial weights_square_array when switched to flag mode when the other object may not that causes a fail when check is called at the end of __add__.

Copy link
Member

@mkolopanis mkolopanis left a comment

Choose a reason for hiding this comment

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

I definitely think I like this version better. If @adampbeardsley could weigh in I would appreciate him taking a look too.

@adampbeardsley
Copy link
Member

Sorry it took so long for me to look at this. Overall it looks good, thanks for putting in the effort Mike!
My only concern is around switching whether weights_square_array is required. I'm not sure the required attribute is meant to change based on whether it exists (admittedly we change it based on the mode, but I think that's more akin to subclassing, which we probably should have done).
I suggest we add the line:

and attr.name != "weights_square_array"

back into clear_unused_attributes, and ensure any weights_square_array is removed when changing to flag mode.
Is that possible, and will it be sufficient for what you want?

@mwilensky768
Copy link
Author

mwilensky768 commented Jun 1, 2020

@adampbeardsley I believe my newest commits accomplish everyone's goals.

Copy link
Member

@adampbeardsley adampbeardsley left a comment

Choose a reason for hiding this comment

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

Thanks @mwilensky768 !

@mwilensky768 mwilensky768 merged commit 1b51565 into master Jun 1, 2020
@mwilensky768 mwilensky768 deleted the UVFlag_add_enhance branch June 1, 2020 22:39
@mkolopanis mkolopanis mentioned this pull request Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants