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

test core_fast added [Issue #28] #30

Closed
wants to merge 1 commit into from
Closed

test core_fast added [Issue #28] #30

wants to merge 1 commit into from

Conversation

ojeda-e
Copy link
Member

@ojeda-e ojeda-e commented Jun 18, 2021

Fixes issue #28

Changes

  • test_core_fast added.
  • removed unused argument in the core_fast function in core.py.
  • removed unnecesary tests functions.

This PR helps to solve #17

@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #30 (0769327) into main (bf6019b) will increase coverage by 4.71%.
The diff coverage is 100.00%.

@ojeda-e
Copy link
Member Author

ojeda-e commented Jun 18, 2021

@IAlibay, can I bother you to review this, pretty please?

@lilyminium lilyminium requested a review from IAlibay June 18, 2021 23:59
@ojeda-e ojeda-e linked an issue Jun 19, 2021 that may be closed by this pull request
Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

As with all the other tests, could you please add an example on a much smaller system that humans can look at and understand? For example:

To check gridding:

  • 25 lipids in 5x5 grid, with x values of 0, 1, 2, 3, 4 and y values of 0, 1, 2, 3, 4
  • 25 lipids in 5x5 grid, with x values of 0.5, 1.5, 2.5, 3.5, 4.5 and y values of 0, 1, 2, 3, 4
    • 25 lipids in 5x5 grid, with y values of 0.5, 1.5, 2.5, 3.5, 4.5 and x values of 0, 1, 2, 3, 4
  • 25 lipids in 5x5 grid, with x values of 0.5, 1.5, 2.5, 3.5, 4.5 and y values of 0.5, 1.5, 2.5, 3.5, 4.5
  • 25 lipids in 5x5 grid, with x values of -0.5, -1.5, 2.5, 3.5, 4.5 and y values of 0, 1, 2, 3, 4
  • so on and so forth
  • 50 lipids in 5x5 grid to check gridding of multiple lipids
  • 50 lipids in 5x5 grid but all on one side of the box (like x > 2) to check gridding of uneven lipids

To check z-coord averaging

  • z values of all 0
  • z values of -1 and 1 (should average to 0 for the 50 lipid system, to check gridding)
  • z values at the top of the box and bottom of the box (should average to 0, to check PBC)

Perhaps the other mentors @orbeckst @fiona-naughton @IAlibay have different perspectives, but IMO you will need to write these tests anyway for the new refactored code, and having them now will be an easy way to remember what does and doesn't work on your existing mdtraj code.

z_calc = np.zeros([n_cells, n_cells])
core_fast_leaflet(z_calc, "upper", mdtraj_po4, jump, n_cells, ["POPC"], md_ref_beads, max_width)
core_fast(mdtraj_po4, jump, n_cells, ["upper"], ["POPC"], md_ref_beads, max_width, 'test')
for z, z_test in zip(MEMBRANE_CURVATURE_DATA['z_avg_coords'], z_calc):
Copy link
Member

Choose a reason for hiding this comment

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

In unit testing we typically try to validate a single function in at least one test. Could you add tests where you check the functionality of core_fast and core_fast_leaflet separately in addition to this one?

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.

Write test for core_fast in mods.py
2 participants