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

Sigmoidal Contrast #1681

Conversation

jamesmcclain
Copy link
Member

@jamesmcclain jamesmcclain commented Oct 20, 2016

Still needs:

Original:
screenshot from 2016-10-20 09 29 00

Imagemagick (convert -sigmoidal-contrast 10,50%):
screenshot from 2016-10-20 09 12 14

This PR (with alpha=0.5 and beta=10):
screenshot from 2016-10-20 09 12 22

@lossyrob lossyrob added this to the 1.0 milestone Oct 20, 2016
@jamesmcclain jamesmcclain force-pushed the feature/jamesmcclain/sigmoidal-contrast branch 4 times, most recently from de11bcc to a59a04c Compare October 24, 2016 02:50
@lossyrob
Copy link
Member

There's a checkbox on the "Still needs" missing, is this ready to go?

@jamesmcclain
Copy link
Member Author

There's a checkbox on the "Still needs" missing, is this ready to go?

@lossyrob No, I still need unit tests for the RDD case. I am writing those right now.

@jamesmcclain jamesmcclain force-pushed the feature/jamesmcclain/sigmoidal-contrast branch from a59a04c to 32deaad Compare October 24, 2016 13:34
@jamesmcclain
Copy link
Member Author

Okay, unit tests have been added.

* @return The output tile
*/
def apply(tile: Tile, alpha: Double, beta: Double): Tile = {
val T = _T(tile.cellType, alpha, beta)_
Copy link
Member

Choose a reason for hiding this comment

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

upper-case vals go against Scala style norms. Is there a specific reason to use it here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, will change.


private def _T(
cellType: CellType, alpha: Double, beta: Double
)(_u: Double): Double = {
Copy link
Member

Choose a reason for hiding this comment

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

using an underscore in a param name is out of normal scala style - is there reason to use it? Could we replace this with a longer, more descriptive name instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, will-do.

def multiband[K, V: (? => MultibandTile): ClassTag, M](
rdd: RDD[(K, V)] with Metadata[M],
alpha: Double, beta: Double
): RDD[(K, MultibandTile)] with Metadata[M] = {
Copy link
Member

Choose a reason for hiding this comment

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

Do these need the Metadata? or could we just make these methods on RDD[(K, MultibandTile)]? We want to create methods on the least-specified versions (only asking for type restrictions that we need) so that we can utilize this for as broad of a range of use cases as possible. For instance, could we run this on source tiles of type RDD[(ProjectedExtent, MultibandTile)]?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I will give that a try.

@jamesmcclain
Copy link
Member Author

I think that all comments prior to this one have been addressed.

@lossyrob lossyrob merged commit 320f51e into locationtech:master Oct 25, 2016
@jamesmcclain jamesmcclain deleted the feature/jamesmcclain/sigmoidal-contrast branch October 25, 2016 15:02
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.

2 participants