search

coarse-graining / cgnet

learning coarse-grained force fields
BSD 3-Clause "New" or "Revised" License
57 stars 26 forks source link

suggestions for get-idx #56

Closed brookehus closed 5 years ago

brookehus commented 5 years ago

The main thing I did here is make order an attribute of ProteinBackboneStatistics (also renamed previous attribute _order to avoid confusion). The order is determined automatically based on what you calculate. I also removed order as an argument from get_zscores and get_bond_constants, so you have to stick with the order you started with when you instantiated the class.

The inspiration for doing this was that in your tests you rely on the ordering of keys in a dictionary, which is not a safe thing to do.

What do you think?

brookehus commented 5 years ago

These are suggestions for #54 to be addressed before dealing with #55

brookehus commented 5 years ago

I also added 2 tests to replace yours - let me know if you think the tests are missing something that yours had.

If you're OK to merge this into #54, then we can merge #54 into master, pull master into #55, and then check out where that leaves us with #55.

brookehus commented 5 years ago

Ok, I've tried to also incorporate what you were getting at in #55. I moved your code to a test. Let me know what you think?

brookehus commented 5 years ago

I made _get_redundant_distance_mapping a hidden function that is called if get_redundant_distance_mapping is set to True (default) in the initialization. Can be turned off if not needed. We can also make the default false if that is better? What do you think?

brookehus commented 5 years ago

I think the steps should be

  1. Merge #56 into #54 (if you like it); close #55 without merging
  2. Add documentation/perhaps some better variable names to test_redundant_distance_mapping_vals
  3. Merge to master when we're both good with it
nec4 commented 5 years ago

LGTM. All tests pass.