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

Clean up util.num.make_symmetric_matrix_from_upper_tri and util.io_util.micro_pyawk, fix new ruff errors #4192

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Nov 23, 2024

Simplify/clarify util.num.make_symmetric_matrix_from_upper_tri and add tests

Caution

make_symmetric_matrix_from_upper_tri now raise ValueError for non 3x3 array

I believe utils.num.make_symmetric_matrix_from_upper_tri is designed to work on 3x3 matrix alone (the new index is hard-coded):

idx = [0, 3, 4, 1, 5, 2]
val = np.array(val)[idx]

As such I might greatly simplify the implementation f2c3252 which gives ~3x speedup and much better readability.

Clean up util.io_util.micro_pyawk

Warning

The debug/postdebug is likely debug args, no usage across the entire code bass, schedule them for removal after one-year grace period in case someone really need them.

Others

  • Remove:

    umask = os.umask(0)
    os.umask(umask)

    I believe this is doing nothing (set umask to 0, but reset it back the original umask immediately)

  • Fix new ruff errors and bump pre-commit accordingly

"""
idx = [0, 3, 4, 1, 5, 2]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this helper function is designed to work for 3x3 matrix alone, as such I might just make things simple and reject non 3x3 arrays with ValueError.

The new implementation gives a ~3 speedup:

Original implementation time: 0.378586 seconds
Simplified implementation time: 0.118705 seconds

Test script (from GPT):

import numpy as np
from timeit import timeit

def original_make_symmetric_matrix_from_upper_tri(val):
    """Original implementation."""
    idx = [0, 3, 4, 1, 5, 2]
    val = np.array(val)[idx]
    mask = ~np.tri(3, k=-1, dtype=bool)
    out = np.zeros((3, 3), dtype=val.dtype)
    out[mask] = val

    out.T[mask] = val
    return out

def simplified_make_symmetric_matrix_from_upper_tri(val):
    """Simplified implementation."""
    array = np.asarray(val)
    if array.shape != (6,):
        raise ValueError(f"Expected val of length 6, got {array.shape}")

    return np.array([
        [array[0], array[3], array[4]],
        [array[3], array[1], array[5]],
        [array[4], array[5], array[2]],
    ])

# Test input
val = [1, 2, 3, 4, 5, 6]

# Measure execution time
original_time = timeit(
    "original_make_symmetric_matrix_from_upper_tri(val)",
    globals=globals(),
    number=100000,
)
simplified_time = timeit(
    "simplified_make_symmetric_matrix_from_upper_tri(val)",
    globals=globals(),
    number=100000,
)

# Display results
print(f"Original implementation time: {original_time:.6f} seconds")
print(f"Simplified implementation time: {simplified_time:.6f} seconds")

@DanielYang59 DanielYang59 marked this pull request as ready for review November 23, 2024 04:39
@DanielYang59 DanielYang59 changed the title Clean up util.num.make_symmetric_matrix_from_upper_tri and util.io_util.micro_pyawk Clean up util.num.make_symmetric_matrix_from_upper_tri and util.io_util.micro_pyawk, fix new ruff errors Nov 23, 2024
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.

1 participant