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

Moral degenerates #20

Merged
merged 5 commits into from May 15, 2016
Merged

Moral degenerates #20

merged 5 commits into from May 15, 2016

Conversation

sgillies
Copy link
Member

@sgillies sgillies commented Apr 7, 2016

Closes #18

Sean Gillies added 2 commits April 7, 2016 16:57
Add optional precision argument to predicates.

Also use the system's float_info.precision to detect degeneracy.
Also add a higher precision test of almost_equals.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 97.333% when pulling fcfbe8f on moral-degenerates into b225255 on master.

@sgillies
Copy link
Member Author

sgillies commented Apr 7, 2016

@perrygeo I'm offering this one as an alternative to #19. This PR doesn't touch much of the implementation except to turn predicate properties that were using the global epsilon into methods that take a precision keyword argument (defaulting to 1.0e-5).

It also removes any precision from the degeneracy test, now it's just self.determinant == 0.0 and that seems to work fine.

I'm putting this on a 2.0 milestone because it's an API breaker.

@sgillies sgillies added this to the 2.0 milestone Apr 7, 2016
@perrygeo
Copy link

perrygeo commented Apr 7, 2016

This looks great. Def. a breaking change since many properties become methods. But I think it's a good move.

I wonder if we could apply the degeneracy test of self.determinant == 0.0 to the 1.2.x branch since that solves the original issue of certain raster affines being uninvertible with decimal degree units.

Also a left field, totally wacky idea to maintain backwards compatibility. I wonder if there is a way to create a hybrid property-method such that self.prop would dispatch to self.prop() and self.prop(**kwargs) would just work.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.6%) to 100.0% when pulling b7e3c59 on moral-degenerates into b225255 on master.

@sgillies
Copy link
Member Author

sgillies commented Apr 7, 2016

Bonus 100% coverage :)

@perrygeo
Copy link

FWIW, I did find a way to make properties callable so that we could keep using them as properties or optionally as methods. Via @kezabelle : https://gist.github.com/kezabelle/43f45edefdcd1377b70eb171d4cffd1e#file-property_method-py

Not sure if this is something you'd be interested in implementing for the sake of backwards compat? But it proves that it can be done. Leaving it here just in case.

@sgillies
Copy link
Member Author

@perrygeo I like it!

@sgillies sgillies mentioned this pull request May 15, 2016
Can be assigned as an instance attribute if user needs a non-default
value.
Conflicts:
	affine/__init__.py
@coveralls
Copy link

coveralls commented May 15, 2016

Coverage Status

Coverage increased (+1.9%) to 100.0% when pulling 4110dac on moral-degenerates into 0cd1cf2 on master.

@sgillies
Copy link
Member Author

In the end, I think it's best for precision to be a Affine instance attribute and not add a complex descriptor.

@sgillies sgillies closed this May 15, 2016
@sgillies sgillies deleted the moral-degenerates branch May 15, 2016 19:06
@sgillies sgillies restored the moral-degenerates branch May 15, 2016 19:08
@sgillies sgillies reopened this May 15, 2016
@sgillies sgillies merged commit b4234ea into master May 15, 2016
@sgillies sgillies deleted the moral-degenerates branch May 15, 2016 19:10
@sgillies
Copy link
Member Author

Please ignore the noise caused by my fumbling fingers.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.9%) to 100.0% when pulling 4110dac on moral-degenerates into 0cd1cf2 on master.

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

3 participants