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

Fix wrong setup of LJ, improve CI and coverage #63

Merged
merged 24 commits into from
Jun 3, 2024

Conversation

pm-blanco
Copy link
Collaborator

@pm-blanco pm-blanco commented May 20, 2024

Quite dense PR, I would appreciate at least two reviewers. Thank you @mariusaarsten for the help while addressing this PR. Partially solves #58.

Bug fixes:

  • closes Fix issue on create_paper_data.py #59
  • Fixes a heavy bug introduced in Refactor lj parameters #27. In that PR, we replaced diameter by sigma, but we did not correct the setup for the sample scripts with particles with different sizes. I have added a proper offset to keep the intended previous behaviur.
  • Different behaviors were obtained depending on which order pmb.load_pka_set and pmb.load_interaction_parameters were called. Now one gets consistent behavior independently of the order.
  • pmb.read_protein_vtf_in_df returned the estimated diameter topology_dict["radius"] instead than the radius
  • Removes the Pandas warnings from lib/create_cg_from_pdb.py

Code improvements:

  • I have deprecated the following protein-specific functions:
  • pymbe_library.define_AA_particles_in_sequence()
  • pymbe_library.define_epsilon_value_of_particles()
  • pymbe_library.define_particles_parameter_from_dict()
  • pymbe_library.setup_particle_sigma()
  • pymbe_library.write_output_vtf_file()
  • I have merged them in a more general function to define the parameters of several particles pymbe_library.define_particles()
  • pymbe_library.write_output_vtf_file() was not really necesary since there already exists a built-in function in espresso that does the same job and furthermore the output vtf were not compatible with visualization/vmd-traj.py.
  • I have removed the case-specific arguments from samples/Beyer2024/globular_protein.py. Now the setup of the metal ions is done automatically inside define_protein. I have also defined new functions pymbe_library.get_metal_ions_charge_map() and pymbe_library.check_if_metal_ion() to deal with the metal ions to make easier later the perspective generalization that we discussed @paobtorres @jngrad

New CI tests for the following functions (some of them very much needed):

  • pymbe_library.calculate_net_charge()
  • pymbe_library.create_added_salt()
  • pymbe_library.create_counterions()
  • pymbe_library.load_interaction_parameters()
  • pymbe_library.check_aminoacid_key()
  • pymbe_library.check_if_metal_ion()
  • pymbe_library.get_metal_ions_charge_map()
  • pymbe_library.define_particles()
  • pymbe_library.define_particle()
  • pymbe_library.define_residue()
  • pymbe_library.define_molecule()
  • pymbe_library.create_particle()
  • pymbe_library.create_residue()
  • pymbe_library.create_molecule()

I ran the functional tests and they all pass. I also ran longer tests and the results seem pretty similar to the results in our publication.

Current coverage: 77% total, 70% in pymbe.py

@pm-blanco pm-blanco self-assigned this May 20, 2024
Copy link
Contributor

@paobtorres paobtorres left a comment

Choose a reason for hiding this comment

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

I tested every file that was changed, and I also reproduce the paper data, everything worked just fine.

@pm-blanco
Copy link
Collaborator Author

@davidbbeyer will you have time to take a look early next week? Otherwise I will simply proceed to merge this PR, this commit has been used by my students for the last two weeks and reviwed by @paobtorres so it seems quite stable 🙂

@davidbbeyer
Copy link
Contributor

@pm-blanco Yes, on Monday.

Copy link
Contributor

@davidbbeyer davidbbeyer left a comment

Choose a reason for hiding this comment

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

Also looks ok for me

@pm-blanco pm-blanco merged commit 55ea6c8 into pyMBE-dev:main Jun 3, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix issue on create_paper_data.py
3 participants