main

CONTRIBUTING.md

@@ -0,0 +1,50 @@
+# Reproduction Process
+
+Observations:
+   - To repoduce the issue, I first commented out the call to get() in line 1487 and replaced it with a call to flag instead.
+   - The terminal blanks and doesn't output anything when the flag() method is called.
+   - I expect the computer to know what to do when it is given a function like flag() and output the correct result.
+
+Environment Setup: I did not face any problems during the environment setup process as I have been using.
+
+Steps to Reproduce:
+   1. Open the file case_validator.py using the file path toolchain/mfc/case_validator.py
+   2. Go to line 1487 in the check_chemistry() method
+   3. Replace the value assigned to the chemistry variable with self.flag("chemistry")
+   4. Reproduce the issue on your terminal by running the following: python3 toolchain/mfc/gen_case_constraints_docs.py | grep "ANALYZER"
+
+Branch Link: https://github.com/mansibrahman03/MFlowCode/tree/main
+
+# Solution Approach
+
+Implementation:
+
+1. At the moment, case_validator.py reads boolean case flags with the verbose form self.get("name", "F") == "T" in 134 places. To clean this code up I want to implement a map function. However, I am prevented from doing so at the moment because of the hard coding in ast_analyzer.py in the _build_local_param_map() method in line 232: call.func.attr == "get". Thus, when the analyzer sees the word "flag", it doesn't recognize it and blanks out.
+
+2. My solution is to first make sure the analyzer recognizes flag() by adding a conditional statement in _build_local_param_map() in ast_analyzer.py. I will then implement a flag() method in case_validator.py that maps the verbose form to a simple boolean value.
+
+3. Files I will touch include:
+   - case_validator.py
+   - ast_analyzer.py
+
+4. I will verify my approach works by calling flag() instead of get() in line 1487 of case_validator.py. If the analyzer correctly returns a value instead of blank, it successfully recognized the flag() function. I will also test the flag() method itself to ensure it correctly returns true or false.
+
+# Implementation Notes
+
+Implementation Progress: I fixed _build_local_param_map() in ast_analyzer.py to recognize flag() method and added a flag() helper method in case_validator.py.
+
+Challenges Faced: The main challenge was understanding the codebase.
+
+Testing Strategy: I tested the flag method itself to ensure it correctly return a bool for all possible params (T, F, null). I also test to see that the old broken code fails and the new code works. 
+
+Branch Link: https://github.com/mansibrahman03/MFlowCode/tree/main
+
+# PR
+
+PR Link: [Direct link to your submitted pull request](https://github.com/MFlowCode/MFC/pull/1622)
+
+PR Description: I added a flag() method in toolchain/mfc/case_validator.py that functions to replace the verbose idiom "self.get(x,F)==T" repeated in 134 places across the file. I also updated ast_analyzer.py to recognize calls made to flag().
+
+Maintainer Feedback: I am currently waiting for maintainer to respond.
+
+Status: Awaiting review

toolchain/mfc/case_validator.py

@@ -246,6 +246,10 @@ def __init__(self, params: Dict[str, Any]):
     def get(self, key: str, default=None):
         """Get parameter value with default"""
         return self.params.get(key, default)
+
+    def flag(self, name: str) -> bool:
+        """True if and only if the case sets `name` to "T"."""
+        return self.get(name, "F") == "T"
 
     def is_set(self, key: str) -> bool:
         """Check if parameter is set (not None and present)"""
@@ -1483,8 +1487,7 @@ def check_chemistry(self):
         is provided. No static validation is performed here - chemistry will fail at
         runtime if misconfigured.
         """
-        # Fetch global chemistry and diffusion flags
-        chemistry = self.get("chemistry", "F") == "T"
+        chemistry = self.flag("chemistry")
         diffusion = self.get("chem_params%diffusion", "F") == "T"
         num_fluids = self.get("num_fluids")
 

toolchain/mfc/params/ast_analyzer.py

@@ -229,12 +229,13 @@ def _build_local_param_map(self, func: ast.FunctionDef) -> Dict[str, str]:
                         isinstance(call.func, ast.Attribute)
                         and isinstance(call.func.value, ast.Name)
                         and call.func.value.id == "self"
-                        and call.func.attr == "get"
+                        and call.func.attr in ("get", "flag")
                         and call.args
                         and isinstance(call.args[0], ast.Constant)
                         and isinstance(call.args[0].value, str)
                     ):
                         param_name = call.args[0].value
+                        print(f"Issue reproduced: {param_name}")
                         for target in node.targets:
                             if isinstance(target, ast.Name):
                                 m[target.id] = param_name

toolchain/mfc/params_tests/test_flag_helper.py

@@ -0,0 +1,76 @@
+"""
+Tests for CaseValidator.flag() and ast_analyzer._build_local_param_map fix.
+"""
+
+import ast
+import unittest
+
+from ..case_validator import CaseValidator
+from ..params.ast_analyzer import CaseValidatorAnalyzer
+
+
+def _param_map(source: str) -> dict:
+    """Parse a single method body and return its local-param map."""
+    wrapped = "class _V:\n" + "\n".join("    " + l for l in source.splitlines())
+    func = ast.parse(wrapped).body[0].body[0]
+    a = CaseValidatorAnalyzer.__new__(CaseValidatorAnalyzer)
+    return a._build_local_param_map(func)
+
+
+class TestFlag(unittest.TestCase):
+
+    def test_true_when_set_to_T(self):
+        self.assertTrue(CaseValidator({"chemistry": "T"}).flag("chemistry"))
+
+    def test_false_when_set_to_F(self):
+        self.assertFalse(CaseValidator({"chemistry": "F"}).flag("chemistry"))
+
+    def test_false_when_absent(self):
+        self.assertFalse(CaseValidator({}).flag("chemistry"))
+
+    def test_only_uppercase_T_is_truthy(self):
+        """Lowercase, integers, and booleans must not count as set."""
+        for val in ("t", "True", 1, True, None):
+            self.assertFalse(CaseValidator({"x": val}).flag("x"), f"Expected False for {val!r}")
+
+    def test_returns_bool(self):
+        self.assertIsInstance(CaseValidator({"x": "T"}).flag("x"), bool)
+
+
+class TestASTCoupling(unittest.TestCase):
+
+    def test_flag_call_recorded_by_analyzer(self):
+        """self.flag('x') must register 'x' in the param map, same as self.get('x','F')=='T'."""
+        m = _param_map("def check(self):\n    chemistry = self.flag('chemistry')")
+        self.assertEqual(m.get("chemistry"), "chemistry")
+
+    def test_get_call_still_recorded(self):
+        """Existing self.get() pattern must not regress."""
+        m = _param_map("def check(self):\n    d = self.get('chem_params%diffusion', 'F') == 'T'")
+        self.assertEqual(m.get("d"), "chem_params%diffusion")
+
+    def test_old_matcher_would_have_missed_flag(self):
+        """Demonstrate the pre-fix bug: attr=='get' silently dropped flag() calls."""
+        source = "def check(self):\n    chemistry = self.flag('chemistry')"
+        wrapped = "class _V:\n" + "\n".join("    " + l for l in source.splitlines())
+        func = ast.parse(wrapped).body[0].body[0]
+
+        broken = {}
+        for node in ast.walk(func):
+            if isinstance(node, ast.Assign):
+                v = node.value.left if isinstance(node.value, ast.Compare) else node.value
+                if isinstance(v, ast.Call) and isinstance(v.func, ast.Attribute):
+                    if v.func.value.id == "self" and v.func.attr == "get":  # old check
+                        for t in node.targets:
+                            if isinstance(t, ast.Name):
+                                broken[t.id] = v.args[0].value
+
+        self.assertNotIn("chemistry", broken, "Old matcher should miss flag() — that was the bug")
+
+        a = CaseValidatorAnalyzer.__new__(CaseValidatorAnalyzer)
+        fixed = a._build_local_param_map(func)
+        self.assertIn("chemistry", fixed, "Fixed matcher must capture flag() calls")
+
+
+if __name__ == "__main__":
+    unittest.main()