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

PR: Remove "colour.io.luts.common.parse_array" definition usage. #596

Merged

Conversation

njwardhan
Copy link
Contributor

This change makes colour more optimized with respect to its LUT I/O capabilities, and also address the issue: #573.

@KelSolaar KelSolaar changed the title Replacing 'parse_array' with 'as_float_array' in the colour codebase PR: Remove "colour.io.luts.common.parse_array" definition usage. Jun 5, 2020
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.

Thanks @njwardhan!

I left a few comments!

Comment on lines 103 to 106
pre_LUT = [parse_array(lines[i]) for i in [1, 2, 4, 5, 7, 8]]
pre_LUT = []
for i in [1, 2, 4, 5, 7, 8]:
tokens = lines[i].split()
pre_LUT.append(as_float_array(tokens))
pre_LUT_padded = []
Copy link
Member

Choose a reason for hiding this comment

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

Should really be something like that:

pre_LUT = as_float_array([lines[i] for i in [1, 2, 4, 5, 7, 8]])

No need to unroll the list comprehension into a loop, unless it is causing issues.

Copy link
Contributor Author

@njwardhan njwardhan Jun 6, 2020

Choose a reason for hiding this comment

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

@KelSolaar, the intention was indeed to keep the code simple and clean. However, the as_float_array has its limitation in doing the direct conversion of a string of numbers to an array of specified type (unlike the parse_array function, which could do it directly). That's why I've used it the way I have. (Obviously instead of changing the actual definition of the as_float_array function).

I am attaching a quick link to a short .ipynb file that should clear my point.
Looking forward to your feedback and suggestions :)

Copy link
Member

@KelSolaar KelSolaar Jun 6, 2020

Choose a reason for hiding this comment

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

Right, you should be able to do that though:

pre_LUT = [as_float_array(lines[i].split()) for i in [1, 2, 4, 5, 7, 8]]

You cannot convert the entire pre_LUT because it is not necessarily uniform though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it @KelSolaar , will update the change. Thankyou!

Comment on lines 124 to 135
size = parse_array(lines[0]).astype(int)
table = np.array([parse_array(line) for line in lines[1:]])
size = []
for i in lines[0].split():
size.append(i)
size = as_int_array(size)

table = []
for line in lines[1:]:
tokens = line.split()
table.append(tokens)
table = as_float_array(table)
Copy link
Member

Choose a reason for hiding this comment

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

Likewise here, same as previously.

Copy link
Member

Choose a reason for hiding this comment

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

E.g.

        size = as_int_array(lines[0].split())
        table = as_float_array([line.split() for line in lines[1:]])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, Thankyou!

Comment on lines 3 to 7
LUT Processing Common Utilities
===============================

Defines LUT Processing common utilities objects that don't fall in any specific
A Common Utility for LUT Processing
====================================
Defines a LUT Processing common utility object that doesn't fall in any specific
category.
"""
Copy link
Member

Choose a reason for hiding this comment

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

There might be more utilities here as you go-on, let's keep it plural for now if you don't mind 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.

Ok sure haha, I didn't think that way. Will make that change! Thankyou!

Comment on lines 66 to 76

Parameters
----------
path : unicode
File path to convert to title.

Returns
-------
unicode
File path converted to title.

Copy link
Member

Choose a reason for hiding this comment

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

Careful with the missing break lines!

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

@@ -83,4 +42,4 @@ def path_to_title(path):
u'ColourCorrect'
"""

return re.sub('_|-|\\.', ' ', os.path.splitext(os.path.basename(path))[0])
return re.sub('_|-|\\.', ' ', os.path.splitext(os.path.basename(path))[0])
Copy link
Member

Choose a reason for hiding this comment

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

Same here, it is missing the end-of-file return.

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

Comment on lines 85 to 86
indexes.append(parse_array(tokens[:3]))
table.append(parse_array(tokens[3:]))
indexes.append(as_float_array(tokens[:3]))
table.append(as_float_array(tokens[3:]))
Copy link
Member

Choose a reason for hiding this comment

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

Careful here! The indexes/indices should be of type int, so you should use as_int_array:

indexes.append(as_int_array(tokens[:3]))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK missed that, will make the change ASAP!

Comment on lines 218 to 216
indexes.append(parse_array(tokens[:3], DEFAULT_INT_DTYPE))
indexes.append(as_float_array(tokens[:3]))
Copy link
Member

Choose a reason for hiding this comment

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

Careful here also! The indexes/indices should be of type int, so you should use as_int_array:

indexes.append(as_int_array(tokens[:3]))

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
Copy link
Member

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

Complexity decreasing per file
==============================
+ colour/io/luts/common.py  -3
         

See the complete overview on Codacy

@KelSolaar
Copy link
Member

LGTM! Thanks @njwardhan!

@KelSolaar KelSolaar merged commit f864731 into colour-science:develop Jun 6, 2020
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants