Skip to content
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.

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

Refactor test for comparing output, do not show plots while testing, no default behavior modified #32

Closed
wants to merge 4 commits into from

Conversation

bobleesj
Copy link
Contributor

@bobleesj bobleesj commented Nov 9, 2024

Context

No code was modified under src

  • refactored tests/test_NMF_analysis_code.py which runs main(arg) with args being passed.
  • prevented from displaying plots while running pytest (the original code does indeed support not showing plots)

Here is the discussion that requires your input: To make the tests pass locally (3 cases), I updated the expected .json outputs (since it's integrated with the cloud, didn't want to change anything under src)

Purpose of this PR

However, it still fails via GitHub Actions, most likely due to convergence problem as displayed in the warnings below or random seed. But, the merit of this PR could be refactoring the test function so that it's easier to debug down the road.

Local test:

========================================== warnings summary ===========================================
tests/test_NMF_analysis_code.py: 19 warnings
  /Users/imac/miniconda3/envs/diffpy.nmf_mapping_env/lib/python3.13/site-packages/sklearn/decomposition/_nmf.py:1759: ConvergenceWarning: Maximum number of iterations 1000 reached. Increase it to improve convergence.
    warnings.warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=================================== 5 passed, 19 warnings in 8.54s ===============================

Copy link

github-actions bot commented Nov 9, 2024

Warning! No news item is found for this PR. If this is a user-facing change/feature/fix,
please add a news item by copying the format from news/TEMPLATE.rst.

def test_nmf_mapping_code(args, output_dir, tmpdir):

# Save the result in ("nmf_result") at the top project level (default behavior)
main(args=args)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tl;dr

This main(args=args) creates 3 .json files under nmf_result at the top directly level, which is the default behavior. I then copy these outputs to tmpdir and then compare those three json files under tmpdir against the expected .json files under tests/output/output1, tests/output/output2, and tests/output/output3.

The three 3 json files are:

    json_files_to_check = [
        "component_index_vs_pratio_col.json",
        "component_index_vs_RE_value.json",
        "x_index_vs_y_col_components.json",
    ]

@bobleesj
Copy link
Contributor Author

bobleesj commented Nov 9, 2024

@sbillinge ready for your insight. Thank you.

@sbillinge
Copy link
Contributor

In general, my impulse here is to junk this test. In the spirit of the discussion around the test for Qmin Qmax in diffpy.fourigui, I have a strong sense that this test just runs the code, pastes the output in a file and then makes sure the output is the same. The question then is "what behavior is this testing?" If the code is suddenly run with different precision on someone's platform and the test fails, is this desired? In other words, from a functional point of view, do we care about precision? The answer is no of course, we just want the code to work and do an nmf decomposition successfully.

This seems to fall into the trap that the test tests the code, not the behavior.

The right way to discuss this is to address the question of what behavior is this function being tested supposed to be implementing? Then let's write a test that captures the different behaviors we want this to have under different situations.

@bobleesj
Copy link
Contributor Author

@sbillinge You are right. This was a refactoring task. So we are still just trying to satisfy our test functions that simply run the code, instead of writing tests to capture the behavior that WE want.

The right way to discuss this is to address the question of what behavior is this function being tested supposed to be implementing? Then let's write a test that captures the different behaviors we want this to have under different situations.

Yes, I probably need to re-visit this code after the Fall 2024 tasks. Or do we have a member who could take over this task?

@bobleesj
Copy link
Contributor Author

Closing: We want to write tests that do not simply fix or refactor the existing test functions that run main and assert the output content. Instead, we aim to test the expected behavior that we want our code to produce

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants