Staging to main: Fix bug in MAP and added new notebook programmatic execution

[miguelgfierro]

Description

Related Issues

References

Checklist:

• I have followed the contribution guidelines and code style for this project.
• I have added tests covering my contributions.
• I have updated the documentation accordingly.
• This PR is being made to staging branch and not to main branch.

[miguelgfierro]

@loomlike I think there are some of the tests in the nightly builds that haven't been updated:

@pytest.mark.notebooks
    @pytest.mark.parametrize(
        "size, expected_values",
        [
            (
                "1m",
                ***
                    "map": 0.060579,
                    "ndcg": 0.299245,
                    "precision": 0.[2701](https://github.com/recommenders-team/recommenders/actions/runs/6749833186/job/18351719718?pr=2035#step:3:2708)16,
                    "recall": 0.104350,
                ***,
            ),
            (
                "10m",
                ***
                    "map": 0.098745,
                    "ndcg": 0.319625,
                    "precision": 0.275756,
                    "recall": 0.154014,
                ***,
            ),
        ],
    )
    def test_sar_single_node_functional(
        notebooks, output_notebook, kernel_name, size, expected_values
    ):
        notebook_path = notebooks["sar_single_node"]
        pm.execute_notebook(
            notebook_path,
            output_notebook,
            kernel_name=kernel_name,
            parameters=dict(TOP_K=10, MOVIELENS_DATA_SIZE=size),
        )
        results = sb.read_notebook(output_notebook).scraps.dataframe.set_index("name")[
            "data"
        ]
    
        for key, value in expected_values.items():
>           assert results[key] == pytest.approx(value, rel=TOL, abs=ABS_TOL)
E           assert 0.1850985029206066 == 0.060579 ± 5.0e-02
E             comparison failed
E             Obtained: 0.1850985029206066
E             Expected: 0.060579 ± 5.0e-02

[anargyri]

Does this mean we now get something between -6.38 or -6.51? It looks a bit strange, since nothing changed with explained variance, doesn't it?

[loomlike]

I think this happened when I copied expected outputs values from the fixture over to each tests. This shouldn't be changed. I'll update codes back to what we had. Thank you for the good catch!

[anargyri]

Same question here. So both spark and python evaluations now give slightly different results. Maybe something changed in the input dataframes?

[loomlike]

this should use the same TOL for the consistency.

[loomlike]

@miguelgfierro To fix this issue, I need to change map_at_k() in the notebooks to use map() which will cause conflicts with your changes in PR #2031 where you're removing glue parts:

# Record results with papermill for tests
import scrapbook as sb
sb.glue("map", rank_eval.map_at_k())  --> rank_eval.map()
...

So I think it will be good to wait for your PR gets merged into staging first and then fix the issues if that's okay.

[miguelgfierro]

So I think it will be good to wait for your PR gets merged into staging first and then fix the issues if that's okay.

Got it, makes sense

[loomlike]

@SimonYansenZhao Hi Simon, I found an issue at execute_notebook function I wanted you to confirm:

        if (
            "tags" in cell.metadata
            and "parameters" in cell.metadata["tags"]
            and cell.cell_type == "code"
        ):
            cell_source = cell.source
            modified_cell_source = (
                cell_source  # Initialize a variable to hold the modified source
            )
            for param, new_value in parameters.items():
                if (
                    isinstance(new_value, str)
                    and not (new_value.startswith('"') and new_value.endswith('"'))
                    and not (new_value.startswith("'") and new_value.endswith("'"))
                ):
                    # Check if the new value is a string and surround it with quotes if necessary
                    new_value = f'"{new_value}"'
                # # Check if the new value is a string and surround it with quotes if necessary
                # if isinstance(new_value, str):
                #     new_value = f'"{new_value}"'
                # Define a regular expression pattern to match parameter assignments and ignore comments
                pattern = re.compile(
                    rf"(\b{param})\s*=\s*([^#\n]+)(?:#.*$)?",
                    re.MULTILINE
                    # rf"\b{param}\s*=\s*([^\n]+)\b"
                )
                modified_cell_source = pattern.sub(rf"\1 = {new_value}", cell_source)  <-------- Here.

In the code above, modified_cell_source = pattern.sub(rf"\1 = {new_value}", cell_source) this part always replaces a new value with the original cell_source. This means that the next parameter value is updated into the original cell source string again, not the one we applied the previous value to, which is modified_cell_source. And thus modified_cell_source will only contain the last parameter value that was updated.

To test and fix this, I changed the code to apply the pattern to modified_cell_source cumulatively, and added a test case (to do so, I pull that part into a separate function, as I suggested earlier).

If you see those changes make sense or have any comments, please let me know!
The changes are in jumin/fix_nightly branch.

[loomlike]

Tests are all greeeeeeen!

[miguelgfierro]

Vamos!!!!