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

Started tests for gnm. #803

Merged
merged 1 commit into from
Apr 2, 2016
Merged

Started tests for gnm. #803

merged 1 commit into from
Apr 2, 2016

Conversation

Endle
Copy link
Contributor

@Endle Endle commented Mar 26, 2016

Related to #743

I followed commit 33e15b6 , and added a test for gnm.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

Signed-off-by: Zhenbo Li [email protected]

'DCD parser not available. Are you using python 3?')
def setUp(self):
self.tmpdir = tempdir.TempDir()
self.universe = MDAnalysis.Universe(PSF, DCD)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would the test make sense on MDAnalysisTests.datafiles.XTC rather than DCD? It would allow to run the test on python 3.

@Endle
Copy link
Contributor Author

Endle commented Mar 27, 2016

Thanks.
In my new patch, I used GRO+XTC instead of PSF+DCD, which works fine on Python3.
Also, I moved self.gnm = MDAnalysis.analysis.gnm.GNMAnalysis

del self.tmpdir

def test_gnm(self):
self.gnm = MDAnalysis.analysis.gnm.GNMAnalysis(self.universe)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not need to be in the instance namespace: self.gnm could be just gnm. Then it is not needed to delete it latter as it will just not live out of the function scope.

@Endle
Copy link
Contributor Author

Endle commented Mar 28, 2016

Does not need to be in the instance namespace: self.gnm could be just gnm. Then it is not needed to delete it latter as it will just not live out of the function scope.

Thank you for mentioning it.

@jbarnoud
Copy link
Contributor

Your test covers about 60% of the gnm module. Do you want to tackle what is left? Or at least increase the coverage? As it is now, it is a good start but the module cannot be considered supported yet.

Otherwise, it looks good to me.

@Endle
Copy link
Contributor Author

Endle commented Mar 28, 2016

Thanks.
I'm going to add tests for generate_kirchoff and backup_file
I'll keep contributing these tests to help me understand the code, and make my porting job easier (if my proposal is accepted)

Will you merge this PR, or wait for me to write a (mostly) completed test for gnm, and to send a new PR?

@jbarnoud
Copy link
Contributor

On 28/03/16 13:20, Zhenbo Li wrote:

Thanks.
I'm going to add tests for generate_kirchoff and backup_file
I'll keep contributing these tests to help me understand the code, and
make my porting job easier (if my proposal is accepted)

Will you merge this PR, or wait for me to write a (mostly) completed
test for gnm, and to send a new PR?


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#803 (comment)

If you continue to work on the same tests, I will wait. Add "[WIP]" at
the beginning of the PR title to avoid that the PR get merged by accident.

@Endle Endle changed the title Started tests for gnm. [WIP] Started tests for gnm. Mar 28, 2016
@Endle Endle changed the title [WIP] Started tests for gnm. Started tests for gnm. Mar 28, 2016
@Endle Endle changed the title Started tests for gnm. [WIP]Started tests for gnm. Mar 28, 2016
@Endle
Copy link
Contributor Author

Endle commented Mar 29, 2016

I added some tests, and coverage reached 80%, leaving backup_file, which is not needed to be tested.

I added tests for closeContactGNMAnalysis, but such test is very slow. It costed 294.926s on my computer(before adding it, it costs me only 15.524s). So I added @attr('slow') for it.

Can this PR be merged now?

@Endle Endle changed the title [WIP]Started tests for gnm. Started tests for gnm. Mar 29, 2016
@jbarnoud
Copy link
Contributor

jbarnoud commented Apr 1, 2016

Great! This seems mergeable. There is a few cases you miss and that do not seem difficult to test:

  • GNMAnalysis.run with skip != 1
  • GNMAnalysis with ReportVector set
  • closeContactGNMAnalysis with MassWeigt == False

Are you willing to give them a try?

Signed-off-by: Zhenbo Li <[email protected]>
@Endle
Copy link
Contributor Author

Endle commented Apr 2, 2016

Tests for these situations added.

@richardjgowers richardjgowers merged commit ee9210f into MDAnalysis:develop Apr 2, 2016
@IAlibay IAlibay mentioned this pull request Dec 18, 2023
5 tasks
orbeckst added a commit that referenced this pull request Dec 21, 2023
Update of AUTHORS and CHANGELOG with inferred author contributions.

*  Removed duplicate mattwthompson in 0.20.0 changelog entry.: mattwthompson was placed twice by accident, this removes this duplication.

* Addition of missing authors.

   An retrospective analysis of the authors found via `git shortlog -s -n --email --branches="develop"` found several commits by authors which were not present in the `AUTHORS.md` file.

    - Zhenbo Li: commited via PR: Started tests for gnm. #803 and Make Travis run tests on OSX. #771,
    - Jenna M. Swarthout via PR Update CoC according to suggestions from current CoC committee #4289 and Point to new reporting form link (owned by [email protected]) #4298,
    - Bradley Dice via PR   Fix GSD Windows compatibility #2384 ,
    - David Minh via PR #2668

   There seemed to be no indications in the above mentioned PRs that the author did not want to be in the authors file, it looks like it was just forgotten.

* Addition of missing entries from the changelog

   Continued cross referencing of the git shortlog output but also accounting for the changelog identified several individuals that had not been included in the changelog entries for the release they contributed under. They were added to the relevant entry of the changelog based on the merge commit date.

   Please note that for Tone Bengsten, we a) had no github handle (so they were assigned @tbengtsen), and b) no specific commit. Given we know that this individual was including alongside the encore merge, they were assigned to the 0.16.0 release.

* Update CHANGELOG
* PR #1218
* PR #1284 and #1408
* PR #4109
* based on git history
* PRs #803 and #771 (author addition, changelog addition)
* PR #2255 and #2221
* PR #1225
* PR #4289 and #4298
* PR #4031
* PR #4085
* PR #3635
* PR #2356
* PR #2559
* No GH handle - Encore author addition @tbengtsen
* PR #4184
* PR #2614
* PR #2219
* PR #2384
* PR #2668
* Add missing entry for Jenna

Thanks to @fiona-naughton for helping out with digging into this data :)

Co-authored-by: Fiona Naughton <[email protected]>
Co-authored-by: Oliver Beckstein <[email protected]>
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