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

[TST] New smoke tests for functions in decay.py #367

Merged
merged 7 commits into from
Jul 24, 2019

Conversation

monicayao
Copy link
Contributor

Addresses part of #335

Changes proposed in this pull request:

  • Added smoke tests for fit_decay and fit_decay_ts
  • Both tests are fashioned in the style of test_smoke_kundu_metrics from test_model_kundu_metrics.py
  • Question for the new tests - should I check the size of the outputs, or is that outside the scope of a smoke test?

Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

Generally very good. Small technical point that I’d like corrected: the numbers generated are ints, not bools. In Python they can be casted to bool implicitly but the type of the matrix is still int.

@monicayao
Copy link
Contributor Author

Generally very good. Small technical point that I’d like corrected: the numbers generated are ints, not bools. In Python they can be casted to bool implicitly but the type of the matrix is still int.

@jbteves ah yes! I was aware of this but since 0 and 1s have the same effect as false and trues I thought it would be fine to do this and add a comment to explain it.

Since it is called from the line data = data[mask] in decay.py, the effects are the same. Should I still change this?

@jbteves
Copy link
Collaborator

jbteves commented Jul 19, 2019

Oh, I just want the comment to not say bool, you don’t have to change the test. The most common descriptor is probably “binary mask.”

@codecov
Copy link

codecov bot commented Jul 19, 2019

Codecov Report

Merging #367 into master will decrease coverage by 31.41%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #367       +/-   ##
===========================================
- Coverage   79.48%   48.06%   -31.42%     
===========================================
  Files          40       39        -1     
  Lines        2208     2251       +43     
===========================================
- Hits         1755     1082      -673     
- Misses        453     1169      +716
Impacted Files Coverage Δ
tedana/tests/test_decay.py 100% <100%> (ø) ⬆️
tedana/viz.py 9.86% <0%> (-82.9%) ⬇️
tedana/selection/_utils.py 25% <0%> (-67.5%) ⬇️
tedana/decomposition/_utils.py 27.77% <0%> (-66.67%) ⬇️
tedana/metrics/kundu_fit.py 29.05% <0%> (-66.49%) ⬇️
tedana/decomposition/pca.py 14.77% <0%> (-65.91%) ⬇️
tedana/selection/tedica.py 23.52% <0%> (-64.71%) ⬇️
tedana/selection/tedpca.py 12.9% <0%> (-54.84%) ⬇️
tedana/workflows/tedana.py 9.46% <0%> (-53.91%) ⬇️
tedana/decomposition/ica.py 26.92% <0%> (-53.85%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26fece1...0e03783. Read the comment docs.

jbteves
jbteves previously approved these changes Jul 19, 2019
Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

LGTM.

@tsalo
Copy link
Member

tsalo commented Jul 22, 2019

@monicayao Is there anything else you wanted to do here or is this ready to merge?

@tsalo
Copy link
Member

tsalo commented Jul 23, 2019

Can you pull from master to run the new testing setup?

@monicayao
Copy link
Contributor Author

@tsalo Hi! I think this is ready to merge. I do want to make the random inputs a pytest fixture eventually, but I think that can come after writing the more important test functions first. I updated my branch from master, and everything ran as before. 🎉

@tsalo tsalo merged commit 03ec6a2 into ME-ICA:master Jul 24, 2019
handwerkerd pushed a commit to handwerkerd/tedana that referenced this pull request Sep 11, 2019
* Smoke tests for both functions in decay.py

* Made the comment more clear - binary masks

* Add little comments for sections of smoke and to do for other tests
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