Repository: pyccel/pyccel
Pull request #2041 (2024-11-06 14:10:50)

Extend the C min and max functions to more than two variables
This PR aims to extend the C support for min and max functions to more than two variables.
It leverages the `stc/common.h` functionalities to define two variadic functions for min and max in `stdlib/STC_EXTENSIONS/Common_extensions.h`.
This PR also fixed the import scheme for extension headers:
Previously, stc headers and extension headers were included separately as follows:
```C
#ifndef _HSET_INT64_T
#define _HSET_INT64_T
#define i_type hset_int64_t
#define i_key int64_t
#include <stc/hset.h>
#endif // _HSET_INT64_T

#ifndef _TOOLS_SET_HSET_INT64_T
#define _TOOLS_SET_HSET_INT64_T
#define i_type hset_int64_t
#define i_key int64_t
#include <Set_extensions.h>
#endif // _TOOLS_SET_HSET_INT64_T
```
this is problematic because at the end of `stc` headers, we find a call to `#include "priv/template2.h"`
which undefines what was defined by the header unless `imore` is defined:
```C
#ifdef i_more
#undef i_more
#else
#undef i_TYPE
#undef i_type
#undef i_tag
#undef i_imp
#undef i_opt
#undef i_less
#undef i_cmp
#undef i_eq
#undef i_hash
#undef i_capacity
#undef i_raw_class
....
```
This prohibits the use of these in the extension header.

The proposed solution combines the `stc` header with its respective extension and defines `imore`:
```C
#ifndef _TOOLS_SET_HSET_INT64_T
#define _TOOLS_SET_HSET_INT64_T
#define i_type hset_int64_t
#define i_key int64_t
#define i_more
#include <stc/hset.h>
#include <Set_extensions.h>
#endif // _TOOLS_SET_HSET_INT64_T
```



pyccel-bot[bot]: Hello again! Thank you for this new pull request :star_struck:.

Please begin by requesting your checklist using the command `/bot checklist`

jalalium: /bot check_list
pyccel-bot[bot]: This bot reacts to all comments which begin with `/bot`. This phrase can be followed by any of these commands:
- `show tests` : Lists the tests which can be triggered
- `run X` : Triggers the test X (acceptable values for X can be seen using `show tests`). Multiple tests can be specified separated by spaces.
- `try V X` : Triggers the test X (acceptable values for X can be seen using `show tests`) using Python version V. Multiple tests can be specified separated by spaces, but all will use the same Python version.
- `mark as ready` : Runs the PR tests. If they pass then it adds the appropriate review flag and requests reviews. This command should be used when the PR is first ready for review, or when a review has been answered.
- `commands` : Shows this list detailing all the commands which are understood.
- `trust user X` : Tells the bot that a new user X is trusted to run workflows (prevents misuse of GitHub actions for mining etc). This command can only be used by a trusted reviewer.

Beware: if you have never contributed to this repository and you are not a member of the Pyccel organisation, the bot will ignore all requests to run tests until permitted by a trusted reviewer.

jalalium: bot checklist
jalalium: /bot checklist
jalalium: /bot run tests
pyccel-bot[bot]: There are unrecognised tests.
The following is a list of keywords which can be used to run tests. Tests in bold are run by pull requests when they are marked as ready for review:
- **linux** : Runs the unit tests on a Linux system.
- **windows** : Runs the unit tests on a Windows system.
- **macosx** : Runs the unit tests on a MacOS X system.
- **coverage** : Runs the unit tests on a Linux system and checks the coverage of the tests.
- **docs** : Checks if the documentation follows the numpydoc format.
- **pylint** : Runs pylint on files which are too big to be handled by codacy.
- **pyccel_lint** : Runs a linter to check that Pyccel's best practices are followed.
- **spelling** : Checks if everything in the documentation is spelled (and capitalised) correctly.
- pr_tests : Runs all the tests marked in bold.
- pickle : Checks that .pyccel files have been correctly generated and installed by the installation process.
- editable_pickle : Checks that .pyccel files have been correctly generated and installed by the editable installation process.
- pickle_wheel : Checks that .pyccel files have been correctly generated and packaged into the wheel.
- anaconda_linux : Runs the unit tests on a linux system using anaconda for python.
- anaconda_windows : Runs the unit tests on a windows system using anaconda for python.
- intel : Runs the unit tests on a linux system using the intel compiler.

These tests can be run with the command `/bot run X` (multiple tests can be specified separated by spaces), or with `try V X` to test on Python version V.

pyccel-bot[bot]: It seems like you have forgotten to complete your checklist for this pull request. Please check the list [here](https://github.com/pyccel/pyccel/pull/2041#issuecomment-2459864443), fix the missing items and let me know when everything is ticked and fixed with `/bot mark as ready`.

pyccel-bot[bot]: Unfortunately your PR is not passing the tests so it is not quite ready for review yet. Let me know when it is fixed with `/bot mark as ready`.

jalalium: /bot mark as ready
pyccel-bot[bot]: Unfortunately your PR is not passing the tests so it is not quite ready for review yet. Let me know when it is fixed with `/bot mark as ready`.

EmilyBourne: If we use `i_more` then do we need to call `#undef` ourselves to remove the macros polluting the namespace?
jalalium: > If we use `i_more` then do we need to call `#undef` ourselves to remove the macros polluting the namespace?
The header `#include <stc/priv/template2.h>` that `#undef` was added to the extension headers. So it is taken care of.
jalalium: /bot mark as ready
pyccel-bot[bot]: Unfortunately your PR is not passing the tests so it is not quite ready for review yet. Let me know when it is fixed with `/bot mark as ready`.

jalalium: /bot mark as ready
pyccel-bot[bot]: Unfortunately your PR is not passing the tests so it is not quite ready for review yet. Let me know when it is fixed with `/bot mark as ready`.

jalalium: /bot mark as ready
pyccel-bot[bot]: Hey @pyccel/pyccel-dev ! @jalalium has just created this great new pull request! Check it out and let me know what you think!

pyccel-bot[bot]: @jalalium, @EmilyBourne has a few questions/comments about your code. Can you go through and see if you agree with them. If not go ahead and explain why. Once you've adressed all the comments let me know with `/bot mark as ready` and we will see if we can get approval.

pyccel-bot[bot]: Unfortunately your PR is not passing the tests so it is not quite ready for review yet. Let me know when it is fixed with `/bot mark as ready`.

pyccel-bot[bot]: @EmilyBourne, @jalalium has been working hard and thinks that they have now replied to or fixed all your comments. Could you take another look at the PR and see if you can approve now?

EmilyBourne: /bot run pr_tests

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d598c757..9669e8d7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -50,6 +50,7 @@ All notable changes to this project will be documented in this file.
 -   #738 : Add support for homogeneous tuples with scalar elements as arguments.
 -   Add a warning about containers in lists.
 -   #2016 : Add support for translating arithmetic magic methods (methods cannot yet be used from Python).
+-   #1980 : Extend The C support for min and max to more than two variables
 -   \[INTERNALS\] Add abstract class `SetMethod` to handle calls to various set methods.
 -   \[INTERNALS\] Added `container_rank` property to `ast.datatypes.PyccelType` objects.
 -   \[INTERNALS\] Add a `__call__` method to `FunctionDef` to create `FunctionCall` instances.
diff --git a/pyccel/ast/core.py b/pyccel/ast/core.py
index 7d3c85e3..a71f41e0 100644
--- a/pyccel/ast/core.py
+++ b/pyccel/ast/core.py
@@ -3711,6 +3711,30 @@ class Import(PyccelAstNode):
         else:
             self._target[new_target] = None
 
+    def remove_target(self, target_to_remove):
+        """
+        Remove a target from the imports.
+
+        Remove a target from the imports.
+        I.e., if `imp` is an Import defined as:
+        >>> from numpy import ones, cos
+
+        and we call `imp.remove_target('cos')`
+        then it becomes:
+        >>> from numpy import ones
+
+        Parameters
+        ----------
+        target_to_remove : str | AsName | iterable[str | AsName]
+            The import target(s) to remove.
+        """
+        assert pyccel_stage != "syntactic"
+        if iterable(target_to_remove):
+            for t in target_to_remove:
+                self._target.pop(t, None)
+        else:
+            self._target.pop(target_to_remove, None)
+
     def find_module_target(self, new_target):
         """
         Find the specified target amongst the targets of the Import.
diff --git a/pyccel/codegen/printing/ccode.py b/pyccel/codegen/printing/ccode.py
index 8af2c019..b59bac1e 100644
--- a/pyccel/codegen/printing/ccode.py
+++ b/pyccel/codegen/printing/ccode.py
@@ -14,8 +14,9 @@ from pyccel.ast.basic     import ScopedAstNode
 
 from pyccel.ast.bind_c    import BindCPointer
 
-from pyccel.ast.builtins  import PythonRange, PythonComplex
+from pyccel.ast.builtins  import PythonRange, PythonComplex, PythonMin
 from pyccel.ast.builtins  import PythonPrint, PythonType, VariableIterator
+
 from pyccel.ast.builtins  import PythonList, PythonTuple, PythonSet, PythonDict, PythonLen
 
 from pyccel.ast.builtin_methods.dict_methods  import DictItems
@@ -248,8 +249,14 @@ c_imports = {n : Import(n, Module(n, (), ())) for n in
                  'stdbool',
                  'assert']}
 
-import_header_guard_prefix = {'Set_extensions'  : '_TOOLS_SET',
-                              'List_extensions' : '_TOOLS_LIST'}
+import_header_guard_prefix = {'Set_extensions'    : '_TOOLS_SET',
+                              'List_extensions'   : '_TOOLS_LIST',
+                              'Common_extensions' : '_TOOLS_COMMON'}
+
+
+stc_header_mapping = {'List_extensions': 'stc/vec',
+                      'Set_extensions': 'stc/hset',
+                      'Common_extensions': 'stc/common'}
 
 class CCodePrinter(CodePrinter):
     """
@@ -675,6 +682,32 @@ class CCodePrinter(CodePrinter):
         init = f'{container_name} = c_init({dtype}, {keyraw});\n'
         return init
 
+    def invalidate_stc_headers(self, imports):
+        """
+        Invalidate STC headers when STC extension headers are present.
+
+        This function iterates over the list of imports and removes any targets
+        from STC headers if the target is present in their corresponding
+        STC extension headers.
+        The STC extension headers take care of including the standard
+        headers.
+
+        Parameters
+        ----------
+        imports : list of Import
+            The list of Import objects representing the header files to include.
+
+        Returns
+        -------
+        None
+            The function modifies the `imports` list in-place.
+        """
+        for imp in imports:
+            if imp.source in stc_header_mapping:
+                for imp2 in imports:
+                    if imp2.source == stc_header_mapping[imp.source]:
+                        imp2.remove_target(imp.target)
+
     def rename_imported_methods(self, expr):
         """
         Rename class methods from user-defined imports.
@@ -711,12 +744,13 @@ class CCodePrinter(CodePrinter):
             func = "labs"
         return "{}({})".format(func, self._print(expr.arg))
 
-    def _print_PythonMin(self, expr):
+    def _print_PythonMinMax(self, expr):
         arg = expr.args[0]
         if arg.dtype.primitive_type is PrimitiveFloatingPointType() and len(arg) == 2:
             self.add_import(c_imports['math'])
-            return "fmin({}, {})".format(self._print(arg[0]),
-                                         self._print(arg[1]))
+            arg1 = self._print(arg[0])
+            arg2 = self._print(arg[1])
+            return f"f{expr.name}({arg1}, {arg2})"
         elif arg.dtype.primitive_type is PrimitiveIntegerType() and len(arg) == 2:
             if isinstance(arg[0], Variable):
                 arg1 = self._print(arg[0])
@@ -734,38 +768,22 @@ class CCodePrinter(CodePrinter):
                 self._additional_code += self._print(assign2)
                 arg2 = self._print(arg2_temp)
 
-            return f"({arg1} < {arg2} ? {arg1} : {arg2})"
+            op = '<' if isinstance(expr, PythonMin) else '>'
+            return f"({arg1} {op} {arg2} ? {arg1} : {arg2})"
+        elif len(arg) > 2 and isinstance(arg.dtype.primitive_type, (PrimitiveFloatingPointType, PrimitiveIntegerType)):
+            key = self.get_declare_type(arg[0])
+            self.add_import(Import('stc/common', AsName(VariableTypeAnnotation(arg.dtype), key)))
+            self.add_import(Import('Common_extensions', AsName(VariableTypeAnnotation(arg.dtype), key)))
+            return  f'{key}_{expr.name}({len(arg)}, {", ".join(self._print(a) for a in arg)})'
         else:
-            return errors.report("min in C is only supported for 2 scalar arguments", symbol=expr,
+            return errors.report(f"{expr.name} in C does not support arguments of type {arg.dtype}", symbol=expr,
                     severity='fatal')
 
-    def _print_PythonMax(self, expr):
-        arg = expr.args[0]
-        if arg.dtype.primitive_type is PrimitiveFloatingPointType() and len(arg) == 2:
-            self.add_import(c_imports['math'])
-            return "fmax({}, {})".format(self._print(arg[0]),
-                                         self._print(arg[1]))
-        elif arg.dtype.primitive_type is PrimitiveIntegerType() and len(arg) == 2:
-            if isinstance(arg[0], Variable):
-                arg1 = self._print(arg[0])
-            else:
-                arg1_temp = self.scope.get_temporary_variable(PythonNativeInt())
-                assign1 = Assign(arg1_temp, arg[0])
-                self._additional_code += self._print(assign1)
-                arg1 = self._print(arg1_temp)
-
-            if isinstance(arg[1], Variable):
-                arg2 = self._print(arg[1])
-            else:
-                arg2_temp = self.scope.get_temporary_variable(PythonNativeInt())
-                assign2 = Assign(arg2_temp, arg[1])
-                self._additional_code += self._print(assign2)
-                arg2 = self._print(arg2_temp)
+    def _print_PythonMin(self, expr):
+        return self._print_PythonMinMax(expr)
 
-            return f"({arg1} > {arg2} ? {arg1} : {arg2})"
-        else:
-            return errors.report("max in C is only supported for 2 scalar arguments", symbol=expr,
-                    severity='fatal')
+    def _print_PythonMax(self, expr):
+        return self._print_PythonMinMax(expr)
 
     def _print_SysExit(self, expr):
         code = ""
@@ -857,6 +875,7 @@ class CCodePrinter(CodePrinter):
 
         # Print imports last to be sure that all additional_imports have been collected
         imports = [*expr.module.imports, *self._additional_imports.values()]
+        self.invalidate_stc_headers(imports)
         imports = ''.join(self._print(i) for i in imports)
 
         self._in_header = False
@@ -1033,7 +1052,20 @@ class CCodePrinter(CodePrinter):
             source = source.name[-1].python_value
         else:
             source = self._print(source)
-        if source.startswith('stc/') or source in import_header_guard_prefix:
+        if source == 'Common_extensions':
+            code = ''
+            for t in expr.target:
+                element_decl = f'#define i_key {t.local_alias}\n'
+                header_guard_prefix = import_header_guard_prefix.get(source, '')
+                header_guard = f'{header_guard_prefix}_{t.local_alias.upper()}'
+                code += ''.join((f'#ifndef {header_guard}\n',
+                     f'#define {header_guard}\n',
+                     element_decl,
+                     f'#include <{stc_header_mapping[source]}.h>\n', 
+                     f'#include <{source}.h>\n',
+                     f'#endif // {header_guard}\n\n'))
+            return code
+        elif source.startswith('stc/') or source in import_header_guard_prefix:
             code = ''
             for t in expr.target:
                 class_type = t.object.class_type
@@ -1055,6 +1087,8 @@ class CCodePrinter(CodePrinter):
                         f'#define {header_guard}\n',
                         f'#define i_type {container_type}\n',
                         element_decl,
+                        '#define i_more\n' if source in import_header_guard_prefix else '',
+                        f'#include <{stc_header_mapping[source]}.h>\n' if source in import_header_guard_prefix else '', 
                         f'#include <{source}.h>\n',
                         f'#endif // {header_guard}\n\n'))
             return code
@@ -2630,6 +2664,7 @@ class CCodePrinter(CodePrinter):
         decs = ''.join(self._print(Declare(v)) for v in variables)
 
         imports = [*expr.imports, *self._additional_imports.values()]
+        self.invalidate_stc_headers(imports)
         imports = ''.join(self._print(i) for i in imports)
 
         self.exit_scope()
diff --git a/pyccel/codegen/utilities.py b/pyccel/codegen/utilities.py
index 6e9b0d0e..93067e27 100644
--- a/pyccel/codegen/utilities.py
+++ b/pyccel/codegen/utilities.py
@@ -49,6 +49,7 @@ internal_libs = {
     "numpy_c"         : ("numpy", CompileObj("numpy_c.c",folder="numpy")),
     "Set_extensions"  : ("STC_Extensions", CompileObj("Set_Extensions.h", folder="STC_Extensions", has_target_file = False)),
     "List_extensions" : ("STC_Extensions", CompileObj("List_Extensions.h", folder="STC_Extensions", has_target_file = False)),
+    "Common_extensions" : ("STC_Extensions", CompileObj("Common_Extensions.h", folder="STC_Extensions", has_target_file = False)),
     "gFTL_functions/Set_extensions"  : ("gFTL_functions", CompileObj("Set_Extensions.inc", folder="gFTL_functions", has_target_file = False)),
 }
 internal_libs["cwrapper_ndarrays"] = ("cwrapper_ndarrays", CompileObj("cwrapper_ndarrays.c",folder="cwrapper_ndarrays",
diff --git a/pyccel/stdlib/STC_Extensions/Common_extensions.h b/pyccel/stdlib/STC_Extensions/Common_extensions.h
new file mode 100644
index 00000000..a23c589c
--- /dev/null
+++ b/pyccel/stdlib/STC_Extensions/Common_extensions.h
@@ -0,0 +1,31 @@
+#include <stdarg.h>
+
+static inline i_key c_JOIN(i_key,_min)(size_t count, ...) {
+    va_list args;
+    va_start(args, count);
+    i_key min_value = va_arg(args, i_key);
+    for (size_t i = 1; i < count; ++i) {
+        i_key value = va_arg(args, i_key);
+        if (value < min_value) {
+            min_value = value;
+        }
+    }
+    va_end(args);
+    return min_value;
+}
+
+static inline i_key c_JOIN(i_key, _max)(size_t count, ...) {
+    va_list args;
+    va_start(args, count);
+    i_key max_value = va_arg(args, i_key);
+    for (size_t i = 1; i < count; ++i) {
+        i_key value = va_arg(args, i_key);
+        if (value > max_value) {
+            max_value = value;
+        }
+    }
+    va_end(args);
+    return max_value;
+}
+
+#undef i_key
diff --git a/pyccel/stdlib/STC_Extensions/List_extensions.h b/pyccel/stdlib/STC_Extensions/List_extensions.h
index 96997d1f..da021108 100644
--- a/pyccel/stdlib/STC_Extensions/List_extensions.h
+++ b/pyccel/stdlib/STC_Extensions/List_extensions.h
@@ -2,8 +2,6 @@
 #define LIST_EXTENSIONS_H
 
 
-#define _c_MEMB(name) c_JOIN(i_type, name)
-
 // This function represents a call to the .pop() method.
 // i_type: Class type (e.g., hset_int64_t).
 // i_key: Data type of the elements in the set (e.g., int64_t).
@@ -24,4 +22,5 @@ static inline i_key _c_MEMB(_pull_elem)(i_type* self, intptr_t pop_idx) {
 
 #undef i_type
 #undef i_key
+#include <stc/priv/template2.h>
 #endif
diff --git a/pyccel/stdlib/STC_Extensions/Set_extensions.h b/pyccel/stdlib/STC_Extensions/Set_extensions.h
index 51f4fa33..8b3c8bb0 100644
--- a/pyccel/stdlib/STC_Extensions/Set_extensions.h
+++ b/pyccel/stdlib/STC_Extensions/Set_extensions.h
@@ -2,8 +2,6 @@
 #define SET_EXTENSIONS_H
 #include <stdarg.h>
 
-#define _c_MEMB(name) c_JOIN(i_type, name)
-
 // This function represents a call to the .pop() method.
 // i_type: Class type (e.g., hset_int64_t).
 // i_key: Data type of the elements in the set (e.g., int64_t).
@@ -44,4 +42,5 @@ static inline i_type _c_MEMB(_union)(i_type* self, int n, ...) {
 
 #undef i_type
 #undef i_key
+#include <stc/priv/template2.h>
 #endif


Test patch:
diff --git a/tests/epyccel/test_builtins.py b/tests/epyccel/test_builtins.py
index ddb41067..e74e70bb 100644
--- a/tests/epyccel/test_builtins.py
+++ b/tests/epyccel/test_builtins.py
@@ -109,15 +109,6 @@ def test_min_2_args_f(language):
 
     assert np.isclose(epyc_f(*float_args), f(*float_args), rtol=RTOL, atol=ATOL)
 
-@pytest.mark.parametrize( 'language', (
-        pytest.param("fortran", marks = pytest.mark.fortran),
-        pytest.param("c", marks = [
-            pytest.mark.skip(reason="min not implemented in C for more than 2 args"),
-            pytest.mark.c]
-        ),
-        pytest.param("python", marks = pytest.mark.python)
-    )
-)
 def test_min_3_args(language):
     @template('T', [int, float])
     def f(x : 'T', y : 'T', z : 'T'):
@@ -228,19 +219,10 @@ def test_max_2_args_f(language):
 
     assert np.isclose(epyc_f(*float_args), f(*float_args), rtol=RTOL, atol=ATOL)
 
-@pytest.mark.parametrize( 'language', (
-        pytest.param("fortran", marks = pytest.mark.fortran),
-        pytest.param("c", marks = [
-            pytest.mark.skip(reason="max not implemented in C for more than 2 args"),
-            pytest.mark.c]
-        ),
-        pytest.param("python", marks = pytest.mark.python)
-    )
-)
 def test_max_3_args(language):
     @template('T', [int, float])
     def f(x : 'T', y : 'T', z : 'T'):
-        return min(x, y, z)
+        return max(x, y, z)
 
     epyc_f = epyccel(f, language=language)