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

correct licensing and incorporation of FastMath #49122

Merged
merged 4 commits into from
Nov 21, 2019
Merged

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Nov 14, 2019

this resolves incorrectly licensed code in #49009.

ESSloppyMath is removed in favor of preserving as much of the original
FastMath as possible. Since no additional methods are introduced in
ESSloppyMath, this abstraction is removed.

thanks @jasontedor for assisting to resolve this.

this resolves incorrectly licensed code in elastic#49009.

ESSloppyMath is removed in favor of preserving as much of the original
FastMath as possible. Since no additional methods are introduced in
ESSloppyMath, this abstraction is removed.
@talevy talevy added :Analytics/Geo Indexing, search aggregations of geo points and shapes >refactoring labels Nov 14, 2019
@talevy talevy requested a review from jasontedor November 14, 2019 22:44
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Geo)

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

This is good. Thanks. I think I like a level of indirection on top of FastMath (and make it package private) though so that we aren't tempted to add code to it that doesn't fall under the license at the top of the file. Maybe I'm overthinking it, and I can be convinced otherwise.

@talevy
Copy link
Contributor Author

talevy commented Nov 18, 2019

I totally get the reasoning, but it feels like overkill given where that there is no other code that would be defined in ESSloppyMath, other than these thin wrappers. Would an alternative to introducing a wrapper be moving FastMath to its original package org.math.plot.utils?

@jasontedor
Copy link
Member

I'm fine without the level of indirection if we add a top-level comment to the class that additions/modifications to it should only be those that come from the original source.

@talevy
Copy link
Contributor Author

talevy commented Nov 20, 2019

after working on some other code, I see value in introducing more helper methods like an ESSloppyMath.sin that is backed by SloppyMath.cos. So I will actually change my mind and revert back to your original recommendation, which will help build a home for these helpers

@jasontedor
Copy link
Member

That’s great, thanks!

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left one comment that requires attention before merging, otherwise LGTM.

@talevy
Copy link
Contributor Author

talevy commented Nov 20, 2019

@elasticmachine update branch

@talevy talevy merged commit 782b4f4 into elastic:master Nov 21, 2019
talevy added a commit that referenced this pull request Nov 21, 2019
this resolves incorrectly licensed code in #49009.

ESSloppyMath is made as a wrapper around FastMath.java which is 
not meant to be modified with code beyond the original source
@ywelsch
Copy link
Contributor

ywelsch commented Feb 27, 2020

The backport PR seems to have been merged. I'm therefore removing the backport pending label here. Please shout if this is incorrect

@talevy talevy deleted the asf-49009 branch February 27, 2020 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >refactoring v7.6.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants