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

Add MapGeom equality operator #1893

Merged
merged 8 commits into from Nov 3, 2018

Conversation

2 participants
@registerrier
Contributor

registerrier commented Oct 26, 2018

This PR introduces the __eq__ operator on both WcsGeom and HpxGeom.

MapAxis equality is tested as well as geometry consistency. In the case of WcsGeom we use a a tolerance of 1e-6 for WCS comparison. The latter is obtained with ~astrop.wcs.WCS.compare.

A later PR will add the equality check for map arithmetics.

@registerrier registerrier added this to the 0.9 milestone Oct 26, 2018

@registerrier registerrier self-assigned this Oct 26, 2018

@registerrier registerrier added this to To do in Map analysis via automation Oct 26, 2018

@registerrier registerrier requested a review from cdeil Oct 26, 2018

if self.data_shape != other.data_shape:
return False
result = True

This comment has been minimized.

@cdeil

cdeil Oct 26, 2018

Member

This is a little simpler, no?

for ax1, ax2 in zip(self.axes, other.axes):
    if ax1 != ax2:
        return False

Alternatively if you want to create a local status variable, I think this would be equivalent:

axes_equal = all(ax1 == ax2 for ax1, ax2 in zip(self.axes, other.axes))

This comment has been minimized.

@registerrier

registerrier Oct 31, 2018

Contributor

OK. Changed to first solution

@cdeil

Some suggestions inline.

geom0 = HpxGeom(16, False, "GAL", region=None)
geom1 = HpxGeom(nside, nested, coordsys, region=region)
assert (geom0 == geom1) is result

This comment has been minimized.

@cdeil

cdeil Oct 26, 2018

Member

I would remove the @pytest.mark.parametrize, and just put the lines here:

geom = HpxGeom(16, False, "GAL", region=None)

assert HpxGeom(16, False, "GAL", None) == geom
...

Seems simpler to me.

Otherwise: +1 to this pattern of testing by varying one property at a time.

def __eq__(self, other):
"""Test equality between two `~gammapy.maps.WcsGeom`. Espect tolerance of 1e-6 for `~astropy.wcs.WCS` objects"""
if not isinstance(other, self.__class__):
raise TypeError('Cannot compare WcsGeom with {}'.format(other.__class__))

This comment has been minimized.

@cdeil

cdeil Oct 26, 2018

Member

I think you're supposed to return NotImplemented when implementing __eq__, no?
Can you please check the errors you get and check the Python docs what's recommended?

This comment has been minimized.

@registerrier

registerrier Oct 31, 2018

Contributor

OK. Done

@@ -882,6 +882,20 @@ def __repr__(self):
)
return str_
def __eq__(self, other):
"""Test equality between two `~gammapy.maps.WcsGeom`. Espect tolerance of 1e-6 for `~astropy.wcs.WCS` objects"""

This comment has been minimized.

@cdeil

cdeil Oct 26, 2018

Member

Comment line is too long. IMO "Test equality between two ~gammapy.maps.WcsGeom." doesn't add value and could be removed. The commend about WCS and 1e6 I would put inline above the line where you call the corresponding method.

This comment has been minimized.

@registerrier

registerrier Oct 31, 2018

Contributor

Done

if self.data_shape != other.data_shape:
return False
result = True

This comment has been minimized.

@cdeil

cdeil Oct 26, 2018

Member

Also here: I would not introduce a local state variable "result" that you accumulate. Just check things and return False if not equal one by one, and put return True on the last line.
I think I've seen that coding pattern a lot in Python codes.

This comment has been minimized.

@registerrier

registerrier Oct 31, 2018

Contributor

Done

@@ -296,3 +296,36 @@ def test_get_axis_index_by_name():
assert geom.get_axis_index_by_name("Energy") == 0
with pytest.raises(ValueError):
geom.get_axis_index_by_name("time")
test_axis1 = [MapAxis(nodes=(1,2,3,4), unit='TeV', node_type='center')]

This comment has been minimized.

@cdeil

cdeil Oct 26, 2018

Member

Also here, I would suggest to try and reduce the lines of test code if possible.
We already have many 1000s too much and it's hard to maintain.

Concretely, I think we shouldn't even try to exhaustively test astropy.wcs.WCS properties, but just rely that that compare works, and just have 1 or 2 explicit cases where it's the same and we get True and one where it's off by 1e-3 and False.
And the whole @pytest.mark.parametrize can be removed and the test cases in the test function, if there's only one line for each the test parametrisation doesn't by you anything, no?

Matter of taste - feel free to finish up as you like an merge.

Map analysis automation moved this from To do to In progress Oct 31, 2018

@registerrier

This comment has been minimized.

Contributor

registerrier commented Oct 31, 2018

Implemented some suggested modifications.
Removed the exception raising and replaced it with NotImplemented. The associated tests were removed as well.
@cdeil Merge?

@registerrier

This comment has been minimized.

Contributor

registerrier commented Nov 3, 2018

Unrelated travis fail. Merging now.

@registerrier registerrier merged commit 45caa68 into gammapy:master Nov 3, 2018

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 6 updated code elements – Tests: passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

Map analysis automation moved this from In progress to Done Nov 3, 2018

@cdeil cdeil changed the title from Add MapGeom.__eq__ operator to Add MapGeom equality operator Nov 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment