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

tests for spatial problems #188

Merged
merged 50 commits into from
May 21, 2022
Merged

tests for spatial problems #188

merged 50 commits into from
May 21, 2022

Conversation

giovp
Copy link
Member

@giovp giovp commented May 1, 2022

@MUCDK so far encounter this mypy error due to multiple conftests.py. How do we go about it?

mypy.....................................................................Failed
- hook id: mypy
- exit code: 2

tests/problems/space/conftest.py: error: Duplicate module named "conftest" (also at "tests/conftest.py")
Found 1 error in 1 file (errors prevented further checking)

also I added the data generation step for (more complex) scenarios, however while doing it I realized that this would be needed for the regression tests but must say that in alignment case only case to do it against Paste for only the affine transform . Do you think it still makes sense?

@codecov
Copy link

codecov bot commented May 1, 2022

Codecov Report

Merging #188 (fce788b) into master (dbf0ddf) will increase coverage by 5.85%.
The diff coverage is 77.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
+ Coverage   71.07%   76.93%   +5.85%     
==========================================
  Files          24       24              
  Lines        1943     1981      +38     
  Branches      342      351       +9     
==========================================
+ Hits         1381     1524     +143     
+ Misses        463      350     -113     
- Partials       99      107       +8     
Impacted Files Coverage Δ
moscot/solvers/_output.py 71.81% <66.66%> (-5.96%) ⬇️
moscot/analysis_mixins/_spatial_analysis.py 80.90% <72.50%> (+57.67%) ⬆️
moscot/analysis_mixins/_base_analysis.py 100.00% <100.00%> (+20.00%) ⬆️
moscot/backends/ott/_output.py 94.52% <100.00%> (+1.46%) ⬆️
moscot/problems/space/_alignment.py 86.36% <100.00%> (+26.36%) ⬆️
moscot/problems/space/_mapping.py 94.11% <100.00%> (+41.05%) ⬆️
moscot/problems/_base_problem.py 84.65% <0.00%> (+1.70%) ⬆️
moscot/solvers/_base_solver.py 89.56% <0.00%> (+1.73%) ⬆️
... and 6 more

@MUCDK
Copy link
Collaborator

MUCDK commented May 1, 2022

@MUCDK so far encounter this mypy error due to multiple conftests.py. How do we go about it?

mypy.....................................................................Failed
- hook id: mypy
- exit code: 2

tests/problems/space/conftest.py: error: Duplicate module named "conftest" (also at "tests/conftest.py")
Found 1 error in 1 file (errors prevented further checking)

also I added the data generation step for (more complex) scenarios, however while doing it I realized that this would be needed for the regression tests but must say that in alignment case only case to do it against Paste for only the affine transform . Do you think it still makes sense?

  • Concerning the mypy errors I haven't found an easy workaround yet, maybe @michalk8 knows a solution. Still, I would like to keep the conftest file structure as it is because it's much cleaner this way.
  • Concerning the regression test I am not entirely sure what you mean. Are you saying that for the alignment task test you need the generated data and you want to test the moscot implementation obviously only for the affine transformation and so you are not sure whether this is reason enough to include the code for the data generation here? If so, I would still include it.

@michalk8
Copy link
Collaborator

michalk8 commented May 1, 2022

Concerning the mypy errors I haven't found an easy workaround yet, maybe @michalk8 knows a solution. Still, I would like to keep the conftest file structure as it is because it's much cleaner this way.

Please take a look at: https://docs.pytest.org/en/7.1.x/reference/fixtures.html#conftest-py-sharing-fixtures-across-multiple-files

@MUCDK
Copy link
Collaborator

MUCDK commented May 2, 2022

Concerning the mypy errors I haven't found an easy workaround yet, maybe @michalk8 knows a solution. Still, I would like to keep the conftest file structure as it is because it's much cleaner this way.

Please take a look at: https://docs.pytest.org/en/7.1.x/reference/fixtures.html#conftest-py-sharing-fixtures-across-multiple-files

If I understand correctly there is no alternative to naming them 'conftest.py'

@giovp
Copy link
Member Author

giovp commented May 2, 2022

reading here: python/mypy#4008
it doesn't seem a clear solution exist. I think having mypy running is more important than nested fixtures so I wouldn't mind just putting everything in root conftest.py

@michalk8
Copy link
Collaborator

michalk8 commented May 2, 2022

@giovp @MUCDK think my latest commit should work.

@giovp
Copy link
Member Author

giovp commented May 8, 2022

@MUCDK tests fails because of this

tests/problems/test_general_problem.py:1: in <module>
      from _utils import ATOL, RTOL, Geom_t, TestSolverOutput
  E   ModuleNotFoundError: No module named '_utils'

any idea? the file is exactly the same as in master yet in master it doesn't fail, here it does...

@giovp giovp marked this pull request as ready for review May 9, 2022 12:06
@@ -0,0 +1,186 @@
from typing import Tuple
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should not exist any more. It should be in tests/analysis_mixins/

Copy link
Collaborator

@MUCDK MUCDK left a comment

Choose a reason for hiding this comment

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

please make sure that the file structure in the test folder corresponds to the file structure in the moscot module

moscot/analysis_mixins/_spatial_analysis.py Outdated Show resolved Hide resolved
moscot/analysis_mixins/_spatial_analysis.py Outdated Show resolved Hide resolved
moscot/problems/space/_mapping.py Show resolved Hide resolved
moscot/problems/space/_alignment.py Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/problems/space/test_alignment_problem.py Outdated Show resolved Hide resolved
tests/problems/space/test_mapping_problem.py Outdated Show resolved Hide resolved
assert mp[prob_key].y.data.shape == (n_obs, y_n_var)
assert mp[prob_key].xy[0].data.shape == mp[prob_key].xy[1].data.shape == (n_obs, xy_n_vars)

@pytest.mark.parametrize("var_names", ["0", [], [str(i) for i in range(20)]])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't empty var_names throw? I'd expect if you don't want to use any, None should be passed.

tests/problems/space/test_mapping_problem.py Outdated Show resolved Hide resolved
tests/mixins/test_time_analysis.py Outdated Show resolved Hide resolved
@giovp giovp changed the title test for spatial problems tests for spatial problems May 21, 2022
@giovp giovp merged commit e0a9a97 into master May 21, 2022
@giovp giovp deleted the tests/spatial branch May 21, 2022 08:40
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.

3 participants