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.

Sign up for GitHub

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

Jump to bottom

PR: Add "colour.models.rgb.transfer_functions.log" module. #600

Merged
merged 3 commits into from Jun 16, 2020
Merged

PR: Add "colour.models.rgb.transfer_functions.log" module. #600

merged 3 commits into from Jun 16, 2020

Conversation

njwardhan
Copy link
Contributor

This pull request adds the log.py module to the 'transfer functions' sub-package in colour models, and contains the following functions:

  • log_encoding_Log2
  • log_decoding_Log2

KelSolaar
KelSolaar reviewed Jun 15, 2020
View reviewed changes
Copy link
Member

@KelSolaar KelSolaar left a comment

Choose a reason for hiding this comment

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

A few minor notes but otherwise looks good!

colour/models/rgb/transfer_functions/__init__.py Outdated
@@ -58,6 +58,7 @@
log_encoding_SLog3, log_decoding_SLog3)
from .srgb import eotf_inverse_sRGB, eotf_sRGB
from .viper_log import log_encoding_ViperLog, log_decoding_ViperLog
from .log import log_encoding_Log2, log_decoding_Log2
Copy link
Member

Choose a reason for hiding this comment

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

Given the module does not depend on anything nor anything really depends on it, please import it in alphabetical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted! Will take care of that in the future! 👍

colour/models/rgb/transfer_functions/__init__.py Outdated
@@ -116,6 +117,7 @@
]
__all__ += ['eotf_inverse_sRGB', 'eotf_sRGB']
__all__ += ['log_encoding_ViperLog', 'log_decoding_ViperLog']
__all__ += ['log_encoding_Log2', 'log_decoding_Log2']
Copy link
Member

Choose a reason for hiding this comment

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

Alphabetical ordering here too please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted!

colour/models/rgb/transfer_functions/__init__.py Outdated
@@ -142,7 +144,8 @@
'S-Log3': log_encoding_SLog3,
'T-Log': log_encoding_FilmLightTLog,
'V-Log': log_encoding_VLog,
'ViperLog': log_encoding_ViperLog
'ViperLog': log_encoding_ViperLog,
'Log2': log_encoding_Log2
Copy link
Member

Choose a reason for hiding this comment

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

Likewise :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure :)

colour/models/rgb/transfer_functions/__init__.py Outdated
@@ -152,7 +155,7 @@
'Canon Log 3', 'Canon Log', 'Cineon', 'D-Log', 'ERIMM RGB', 'F-Log',
'Filmic Pro 6', 'Log3G10', 'Log3G12', 'Panalog', 'PLog', 'Protune',
'REDLog', 'REDLogFilm', 'S-Log', 'S-Log2', 'S-Log3', 'T-Log', 'V-Log',
'ViperLog'}**
'ViperLog', 'Log2'}**
Copy link
Member

Choose a reason for hiding this comment

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

Same ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted!

colour/models/rgb/transfer_functions/__init__.py Outdated
@@ -170,7 +173,7 @@ def log_encoding(value, function='Cineon', **kwargs):
'Canon Log 3', 'Canon Log', 'Cineon', 'D-Log', 'ERIMM RGB', 'F-Log',
'Filmic Pro 6', 'Log3G10', 'Log3G12', 'Panalog', 'PLog', 'Protune',
'REDLog', 'REDLogFilm', 'S-Log', 'S-Log2', 'S-Log3', 'T-Log',
'V-Log', 'ViperLog'}**,
'V-Log', 'ViperLog', 'Log2'}**,
Copy link
Member

Choose a reason for hiding this comment

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

And same for the remaining instances (even in the docs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will take care, Thankyou!

colour/models/rgb/transfer_functions/log.py

References
----------

Copy link
Member

Choose a reason for hiding this comment

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

:cite:`TheAcademyofMotionPictureArtsandSciencesa`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thankyou!

colour/models/rgb/transfer_functions/log.py

References
----------

Copy link
Member

Choose a reason for hiding this comment

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

:cite:`TheAcademyofMotionPictureArtsandSciencesb`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thankyou!

colour/models/rgb/transfer_functions/log.py Outdated

Examples
--------
Linear numeric input gets encoded as follows:
Copy link
Member

Choose a reason for hiding this comment

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

gets is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure :)

colour/models/rgb/transfer_functions/tests/test_log.py Outdated

__all__ = [
'TestLogEncoding_Log2',
'TestLogDecoding_Log2',
Copy link
Member

Choose a reason for hiding this comment

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

Extra coma at the end prevents Yapf inlining.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted

docs/colour.models.rst Outdated
@@ -588,6 +588,8 @@ Log Encoding and Decoding
log_decoding_VLog
log_encoding_ViperLog
log_decoding_ViperLog
log_encoding_Log2
Copy link
Member

Choose a reason for hiding this comment

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

Alphabetical ordering please :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, Thankyou!

@KelSolaar KelSolaar changed the title PR: Add "colour/models/rgb/transfer_functions/log.py" module PR: Add "colour.models.rgb.transfer_functions.log" module. Jun 16, 2020
KelSolaar
KelSolaar reviewed Jun 16, 2020
View reviewed changes
colour/models/rgb/transfer_functions/log.py Outdated
@@ -1,9 +1,9 @@
#
"""
Log2 Shaper Implementation
Common Log Encodings
==========================
Copy link
Member

Choose a reason for hiding this comment

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

Careful at title marker length: ========================== should be shorter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, should have taken care. Noted anyways!

KelSolaar
KelSolaar reviewed Jun 16, 2020
View reviewed changes
colour/models/rgb/transfer_functions/log.py Outdated
Comment on lines 70 to 82
The common *Log2* encoding function can be used
to build linear to logarithmic shapers in the
ACES OCIO configuration.

A (48-nits OCIO) shaper having values in a linear
domain, can be encoded to a logarithmic domain:

+-------------------+------------------+
| **Shaper Domain** | **Shaper range** |
+===================+==================+
| [0.002, 16.291] | [0, 1] |
+-------------------+------------------+

Copy link
Member

Choose a reason for hiding this comment

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

Nice! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thankyou! :)

KelSolaar
KelSolaar reviewed Jun 16, 2020
View reviewed changes
colour/models/rgb/transfer_functions/log.py Outdated
domain, can be encoded to a logarithmic domain:

+-------------------+------------------+
| **Shaper Domain** | **Shaper range** |
Copy link
Member

Choose a reason for hiding this comment

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

Title case range for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted!

KelSolaar
KelSolaar reviewed Jun 16, 2020
View reviewed changes
colour/models/rgb/transfer_functions/log.py Outdated
decoded back to linear domain:

+-------------------+------------------+
| **Shaper Range** | **Shaper Domain** |
Copy link
Member

Choose a reason for hiding this comment

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

Careful, the table will break at the column separator are not aligned, sphinx is very picky with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will take care, Thankyou!

@KelSolaar
Copy link
Member

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- colour/models/rgb/transfer_functions/log.py  1
- colour/models/rgb/transfer_functions/tests/test_log.py  3

See the complete overview on Codacy

@KelSolaar KelSolaar merged commit d79d7cd into colour-science:develop Jun 16, 2020
@KelSolaar
Copy link
Member

LGTM! Thanks @njwardhan!

@KelSolaar KelSolaar added this to the v0.3.16 milestone Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KelSolaar KelSolaar KelSolaar left review comments

@nick-shaw nick-shaw nick-shaw left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.3.16
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@njwardhan @KelSolaar @nick-shaw