Skip to content

compiler: Check sympy_type returns a floating point type in the code printer #2515

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

Closed
JDBetteridge wants to merge 2 commits into from
Closed

compiler: Check sympy_type returns a floating point type in the code printer #2515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
457d716
compiler: Check sympy_type returns a floating point type
JDBetteridge Jan 10, 2025
c0a7a73
sympy: Add integer assumption to dimension
JDBetteridge Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions devito/symbolics/printer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@ def compiler(self):
return self._settings['compiler']

def single_prec(self, expr=None):
# Extract the dtype of the expression
dtype = sympy_dtype(expr) if expr is not None else self.dtype
# Check that the dtype is a floating point type
dtype = dtype if np.issubdtype(dtype, np.floating) else self.dtype
Copy link
Contributor

@mloubout mloubout Jan 12, 2025

Choose a reason for hiding this comment

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

I don't think this is valid, this will return "is float" for integer and will break a lot of cases. This is probably only necessary for some limited cases like cos but not in general

Copy link
Contributor Author

@JDBetteridge JDBetteridge Jan 13, 2025

Choose a reason for hiding this comment

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

I disagree a little bit here, the method is called single_prec not is_float. If an expression only containing integers is passed, it is unclear that the result should return anything other than the default, as determined here by self.dtype.

I could see an argument for following the sympy behaviour of returning None if the precision cannot be determined, but this won't change much as bool(None) == bool(False) == False.

In the bigger picture, there are a lot of issues coinciding here, it seems that sympy_dtype isn't quite fit for purpose and the printer is doing some rather rough type inference. Personally, I think home-grown type inference should be avoided here (at least for the purposes of this PR), but I am open to further suggestions.

Copy link
Contributor

@mloubout mloubout Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

the method is called single_prec not is_float

Well, sure, that's the name we used when we made it, but this is effectively an is_float. This merely checks if it needs the float literal.

If an expression only containing integers is passed it is unclear that the result should return anything other than the default, as determined here by self.dtype

self.dtype is only a last resort default for expression that cannot have a type determined, the type of the expression if determined is always the one that should be used irrespective of what that type is.

return dtype in [np.float32, np.float16]

def parenthesize(self, item, level, strict=False):
Expand Down Expand Up @@ -167,12 +170,12 @@ def _print_Abs(self, expr):
# AOMPCC errors with abs, always use fabs
if isinstance(self.compiler, AOMPCompiler):
return "fabs(%s)" % self._print(expr.args[0])
# Check if argument is an integer
if has_integer_args(*expr.args[0].args):
# Check if argument (always exactly one!) is an integer
if has_integer_args(expr.args[0]):
func = "abs"
elif self.single_prec(expr):
elif self.single_prec(expr.args[0]):
func = "fabsf"
elif any([isinstance(a, Real) for a in expr.args[0].args]):
elif isinstance(expr.args[0], Real):
# The previous condition isn't sufficient to detect case with
# Python `float`s in that case, fall back to the "default"
func = "fabsf" if self.single_prec() else "fabs"
Expand Down
5 changes: 3 additions & 2 deletions devito/types/dimension.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,11 @@ def __new__(cls, *args, **kwargs):
This is only necessary for backwards compatibility, as originally
there was no BasicDimension (i.e., Dimension was just the top class).
"""
# Dimensions can only be integers, so pass this assumption to the constructor
if cls is Dimension:
return BasicDimension(*args, **kwargs)
return BasicDimension(*args, integer=True, **kwargs)
else:
return BasicDimension.__new__(cls, *args, **kwargs)
return BasicDimension.__new__(cls, *args, integer=True, **kwargs)

@classmethod
def class_key(cls):
Expand Down
2 changes: 1 addition & 1 deletion tests/test_symbolics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ def test_cos_vs_cosf():

# Doesn't make much sense, but it's legal
c = dSymbol('c', dtype=np.int32)
assert ccode(cos(c)) == "cos(c)"
assert ccode(cos(c)) == "cosf(c)"
Copy link
Contributor

@mloubout mloubout Jan 12, 2025

Choose a reason for hiding this comment

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

so ccode(Abs(c)) will be absf(c) which is incorrect it should be abs(c).

Copy link
Contributor Author

@JDBetteridge JDBetteridge Jan 13, 2025

Choose a reason for hiding this comment

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

You are correct, there is a bug here with your specific example. However, I think I introduced it in my last PR and it is specific to abs/fabsf/fabs, which I will address.

But, in this test, why should the resultant code be cos(c) over cosf(c) when c is an integer type?

Copy link
Contributor

@mloubout mloubout Jan 13, 2025

Choose a reason for hiding this comment

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

Well cos is technically not defined for integers so it will convert to float or double depending on the function used so I guess the test is broken in itself and should use a function that is valid for integers



def test_intdiv():
Expand Down
Loading