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

Merge tests #21

Merged
merged 21 commits into from
Oct 6, 2017
Merged

Merge tests #21

merged 21 commits into from
Oct 6, 2017

Conversation

Islast
Copy link
Collaborator

@Islast Islast commented Oct 3, 2017

This was supposed to be a simple 'tests' pull request, but it looks like I forgot to merge some small changes from ooh! June? So I'll leave this open for @KirstieJane to take a look. 🍳

Copy link
Member

@KirstieJane KirstieJane left a comment

Choose a reason for hiding this comment

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

Heya @Islast - this looks great - I've added a few requests, the biggest being the rethinking of the logic of write_fixtures. But hopefully not too much work! 😄

folder = input('what would you like to name this folder')
write_fixtures(folder, corrmat=corrmat,net_analysis=net_analysis)

if __name__ == '__main__':
Copy link
Member

Choose a reason for hiding this comment

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

I don't like that write_fixtures actually calls overwrite_fixtures by default.

Maybe we could change it so that it calls write_fixtures but if the files are already there it asks you if you want to overwrite them?

from corrmat_from_regionalmeasures import corrmat_from_regionalmeasures
corrmat_from_regionalmeasures("EXAMPLE_DATA/PARC_500aparc_thickness_behavmerge.csv", "EXAMPLE_DATA/500.names.txt", corrmat_path, names_308_style=True)

def Recreate_network_analysis_fixture(folder, corrmat_path):
Copy link
Member

Choose a reason for hiding this comment

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

Mega nitpicky request - can we rename Recreate_network_analysis_fixture and Recreate_correlation_matrix_figure to have lowercase rs at the beginning?

@@ -0,0 +1,42 @@
import unittest
Copy link
Member

Choose a reason for hiding this comment

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

Super picky - please can Regession_tests be lowercase r?


if covars_file:
covars_list = [ line.strip() for line in open(covars_file) ]
with open(covars_file) as f:
covars_list = [ line.strip() for line in f ]
else:
covars_list = []

Copy link
Member

Choose a reason for hiding this comment

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

This is great! Well caught 🙌

@Islast
Copy link
Collaborator Author

Islast commented Oct 4, 2017

Hi @KirstieJane , I've made the changes you suggested 🌺 including add documentation to regression tests #22
further details in the commit messages

@KirstieJane
Copy link
Member

This looks great @Islast - I'm going to merge it in!

I realy love 💖💖💖 the documentation in the code, but I was actually meaning a separate markdown file that explains the flow of testing. The use case is that contributors may come along and want to see what tests are done and where they can help, and an overview of the way tests are implemented would be a useful place to start.

Not important to add for this pull request tough, merging now! 🎉

@KirstieJane KirstieJane merged commit 3adb9ae into WhitakerLab:master Oct 6, 2017
@Islast Islast deleted the testing branch October 11, 2017 08:22
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.

None yet

2 participants