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

increase code coverage in PSA #1006

Closed
jdetle opened this issue Sep 25, 2016 · 11 comments
Closed

increase code coverage in PSA #1006

jdetle opened this issue Sep 25, 2016 · 11 comments
Assignees
Milestone

Comments

@jdetle
Copy link
Contributor

jdetle commented Sep 25, 2016

Hi all,
One issue with code coverage in PSA is the amount of code dedicated to plotting the results of the path similarity analysis. I'd like to get people's opinions on whether this code should exist to as is, or if we should consider moving it to an example notebook outside of the class. In my limited experience with the analysis modules, it seems like the place for demonstrating how to aggregate and plot the data should be documentation, and not the actual code. There are around 300/1925 lines dedicated to plotting in the PSA module.

@tylerjereddy
Copy link
Member

I've been working on increasing coverage of this module. I think there are algorithm issues with some of the path code (see #1005), but I've generally been able to deal with plotting-related coverage by writing unit tests that check for return of a matplotlib object type in i.e., @richardjgowers analysis module. The latter tests don't really check for the correctness of the plot, but if all the number crunching code is also unit tested, that may be ok.

And the streamline visualization code I contributed does nothing but plot from an enduser standpoint, so I think it can be ok.

@orbeckst
Copy link
Member

Just adding context: The issued in PR #1005 is a documentation issue, not a bug as far as I can tell, but @sseyler should be able to clear any remaining doubts.

The plotting in PSA is pretty complicated and it is a good thing to give users some working examples. Just test for returning a matplotlib object as @tylerjereddy said.

@kain88-de
Copy link
Member

we can also test the actual image. When we save a reference image it should be possible to get the matplotlib image as a bitmap that we can compare to a stored reference image.

@jdetle
Copy link
Contributor Author

jdetle commented Sep 26, 2016

^ Heh. Thats kind of one of those letter of the law vs spirit of the law sort of things.

@orbeckst
Copy link
Member

@sseyler can you please have a look and comment? Then we can decide what to do – possibly add more tests so that the code is covered. Our guidelines say that analysis code must have at least 70% coverage or code is moved to MDAnalysis.analysis.legacy.

@orbeckst
Copy link
Member

Ping @sseyler

@sseyler
Copy link
Contributor

sseyler commented Jun 14, 2017

I'll get on that starting tomorrow! (I actually talked to Taylor Colburn today about helping to update the PSA code and contributing on GitHub, as he's also found some issues and points of improvement.)

@orbeckst orbeckst changed the title Question Regarding Code Coverage in PSA increase code coverage in PSA Jul 10, 2017
@orbeckst orbeckst added this to the 1.0 milestone Jul 10, 2017
@orbeckst
Copy link
Member

Once 1.0 comes around, coverage of PSA needs to be up to our standards and have ≥ 70% code coverage (more is obviously better).

@orbeckst
Copy link
Member

@sseyler if you can convince @tcolburn to write some tests then that would be awesome.

@sseyler
Copy link
Contributor

sseyler commented Jul 19, 2017

@orbeckst

Update: I gave Taylor the full overview today so he has most of what's needed to start writing tests for PSA (git, github + PRs, virtualenvs + editable installs, unit testing + pytest, etc.). Hopefully he (and I) will get some more tests done this week.

@orbeckst
Copy link
Member

Ping @sseyler and @tcolburn ...

@orbeckst orbeckst mentioned this issue Aug 14, 2018
4 tasks
orbeckst pushed a commit to sseyler/mdanalysis that referenced this issue Apr 10, 2019
- fix MDAnalysis#1006
- Updated docs for fit_to_reference; added doc for postfix parameter and note about prefix
- Added prefix parameter to Path.run to match optional prefix parameter accepted by fit_to_reference. Updated docs for Path.run() to match Path.fit_to_reference.
- Code to pass prefix parameter in Path.run to Path.fit_to_reference added
- Fixed typo in PSAnalysis.generate_paths referring incorrectly to directory for fitted trajectories as trj_fit rather than fitted_trajs
- Added default_path_basename parameter to PSAnalysis and inits to "path"
- Added default prefix in PSAnalysis for fitted paths and trajectories; fixed save_paths to use default prefix and path basename if prefix and filename are not provided
- test_load now passes pytest
- Updated changelog with fix to PSA
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants