Closed miguelgfierro closed 8 months ago
@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
@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.
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
@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.
Vamos!!!!
Description
Related Issues
References
Checklist:
staging branch
and not tomain branch
.