Skip to content

Conversation

@mloubout
Copy link
Contributor

@mloubout mloubout commented Jan 20, 2025

Fixes #1374

@mloubout mloubout added the API api (symbolics, types, ...) label Jan 20, 2025

@_lower_macro_math.register(SafeInv)
def _(expr):
return (('SAFEINV(a, b)', '(((a) < 1e-12 || (b) < 1e-12) ? (0.0F) : (1.0F / (a)))'),)
Copy link
Contributor

Choose a reason for hiding this comment

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

the 1e-12 bounds should ideally be pulled from the SafeInv's attributes (default to 1e-12, but allow caller/user to pass a different threshold?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a safe threshold. But this is also only needed for single precision, double precision should probably default to standard div (not that we ever run double prec)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, it's just that like that it's a magic number buried deep down inside a compiler pass, so it should rather be pulled from expr itself (can do as a class attribute)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@codecov
Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 92.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 87.31%. Comparing base (fa903e4) to head (d437b25).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
devito/types/basic.py 76.92% 2 Missing and 1 partial ⚠️
devito/finite_differences/differentiable.py 91.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2520   +/-   ##
=======================================
  Coverage   87.31%   87.31%           
=======================================
  Files         238      238           
  Lines       45771    45810   +39     
  Branches     4060     4060           
=======================================
+ Hits        39965    40000   +35     
- Misses       5123     5126    +3     
- Partials      683      684    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

class SafeInv(Differentiable, sympy.core.function.Application):
_fd_priority = 0

def __new__(cls, val, base, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

is this override just to tolerate potential extra **kwargs (evaluate?) ?

'floor': 1,
'ceil': 1
'ceil': 1,
'SafeInv': 10,
Copy link
Contributor

Choose a reason for hiding this comment

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

should you put it futher up, so they remain in descending order?

@mloubout mloubout force-pushed the wood-run-stable branch 3 times, most recently from 03cc79d to 9e8f4df Compare January 20, 2025 16:41
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@mloubout mloubout force-pushed the wood-run-stable branch 2 times, most recently from d8d60e1 to 25c0e63 Compare January 20, 2025 18:11
@mloubout mloubout force-pushed the wood-run-stable branch 2 times, most recently from 8c11f8d to 4955b9e Compare January 21, 2025 04:16
# to avoid out of memory with greedy compilers
try:
cfunction = self.cfunction
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

drop try-except; compiler.py's already throws an exception if compilation fails, and that's what the user should see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@FabioLuporini FabioLuporini left a comment

Choose a reason for hiding this comment

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

GTG

@mloubout mloubout added the examples examples label Jan 21, 2025
@mloubout mloubout merged commit 490a627 into master Jan 21, 2025
31 checks passed
@mloubout mloubout deleted the wood-run-stable branch January 21, 2025 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API api (symbolics, types, ...) examples examples

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Temporary increase in memory when executed pyrevolve.Operator leads to memory error

5 participants