Speed up check_compatibility for relevant writer method #700
Conversation
|
This pull request introduces 1 alert when merging 7de41c7 into 4f00c99 - view on LGTM.com new alerts:
Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog. |
Codecov ReportBase: 91.39% // Head: 91.48% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #700 +/- ##
==========================================
+ Coverage 91.39% 91.48% +0.09%
==========================================
Files 63 63
Lines 5451 5450 -1
==========================================
+ Hits 4982 4986 +4
+ Misses 469 464 -5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
daico007
changed the title
|
On second, I think the simplify check behavior should be the default behavior for the compatibility check step. I will try to modify the method such that I will speed up in most case and only slow down/perform detail check when encounter hard case. |
|
@daico007 I've pretty much dismantled the lammpswriter in the PR I'm currently working on. So maybe we should limit this to just the speedups checks instead of also including these changes directly to the lammpswriter. Let me pull the PR with what I have currently and maybe we can discuss what needs to be included between them. I mostly followed the way it was done with the |
|
Briefly comparing the two, it looks like I didn't actually make the connection maps you generate in this code addition, but otherwise I've covered the rest of these changes. We can discuss the best way to iterate through the connection types in that PR. |
|
Good point, I will revert the LAMMPS stuff |
gmso/core/topology.py
Outdated
| @@ -1387,7 +1375,7 @@ def create_subtop(self, label_type, label): | |||
| new_top.update_topology() | |||
| return new_top | |||
|
|
|||
| def save(self, filename, overwrite=False, **kwargs): | |||
| def save(self, filename, overwrite=False, simplify_check=True, **kwargs): | |||
There was a problem hiding this comment.
I am not sure why this simplify_check is here if we removed it from other sections. Is this an oversight, or is it doing something that I missed?
…to simplify_checks
With the new change to Topology's potential types, the performance of the writing step may be affected by the
check_compatiblitystep (due to a lot ofsympy.sympifycall). Asimplify_checkoption has been added to speed up the process (started with GROMACS top writer). This PR will attempt to add thesimplify_checkoption to other relevant writer and add documentation at theTopology.savemethod.