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

Make numba an optional dependency #349

Merged
merged 2 commits into from Oct 20, 2023
Merged

Make numba an optional dependency #349

merged 2 commits into from Oct 20, 2023

Conversation

robbievanleeuwen
Copy link
Owner

Fixes #348.

@robbievanleeuwen robbievanleeuwen added the build Build System and Dependencies label Oct 20, 2023
@robbievanleeuwen robbievanleeuwen self-assigned this Oct 20, 2023
@robbievanleeuwen robbievanleeuwen marked this pull request as ready for review October 20, 2023 11:54
@robbievanleeuwen robbievanleeuwen merged commit d2720b7 into master Oct 20, 2023
17 checks passed
@robbievanleeuwen robbievanleeuwen deleted the optional-numba branch October 20, 2023 12:27
@@ -55,7 +103,7 @@ def _assemble_torsion(
return k_el, f_el, c_el


@njit(cache=True, nogil=True) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

This impairs performance as compiled code is not cached. Why not directly define a njit decorator when njit cannot be imported. This solution appears to be overcomplex to me.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Now that you mention this, that's an obvious solution thanks! I didn't realise this would impair the caching. If you have time to implement a fix I'd be happy to merge, otherwise I will get to this next week. Cheers!

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, directly define the decorator on line 31:

def njit(**options):
    def decorator(func):
        def wrapper(*args, **kwargs):
            return func(*args, **kwargs)

        return wrapper

    return decorator

Copy link
Contributor

Choose a reason for hiding this comment

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

The rest does not need to be changed, so cache will still work if numba is installed. Ppl download and run once, the functions will be compiled, cached, and reused. Without cache, those functions will be compiled every time when the analysis is performed. Since most ppl do not change the source code, caching could benefit most users I presume.

@@ -648,7 +696,9 @@ def element_stress(

# extrapolate results to nodes, ignore numba warnings about performance
with warnings.catch_warnings():
warnings.simplefilter("ignore", category=NumbaPerformanceWarning)
if USE_NUMBA:
warnings.simplefilter("ignore", category=NumbaPerformanceWarning)
Copy link
Contributor

@TLCFEM TLCFEM Oct 21, 2023

Choose a reason for hiding this comment

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

Change line 638 to:

sig_zxy_mzz_gp = np.zeros((n_points, 2), order='F')
sig_zxy_vx_gp = np.zeros((n_points, 2), order='F')
sig_zxy_vy_gp = np.zeros((n_points, 2), order='F')

can suppress the warning so no need to import those extra utilities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build System and Dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make numba an optional dependency
2 participants