Fix hbond autocorrel - #5195
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5195 +/- ##
===========================================
+ Coverage 92.72% 92.74% +0.01%
===========================================
Files 180 180
Lines 22472 22475 +3
Branches 3188 3187 -1
===========================================
+ Hits 20838 20845 +7
+ Misses 1176 1174 -2
+ Partials 458 456 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
IAlibay
left a comment
There was a problem hiding this comment.
I'm sorry I'm going to block this PR, there's a few things here that are incorrect.
- The original issue is unclear to me, but I assume the idea is to actually implement the class using AnalysisBase.
- This isn't the correct subclass pattern for AnalysisBase. You should look at our docs or other analysis classes to see what should be done. You most definitely shouldn't be re-implementing the
runmethod. - The
client_HydrogenBondAutoCorrelfixture you added to the tests is doing nothing. - The
hbond_autocorrelfixture is also doing nothing. - Your additions to
_slice_trajare incorrect - you're adding code before a docstring for some reason. Also I'm not convinced that indexing the zeroth frame is a reasonable check here. - Your message says you refactored by switching away from AnalysisFromFunction, but you didn't do that at all.
The number of errors / inaccuracies here makes me think you've been using an LLM for this contribution, before you continue, can you confirm if you are doing this?
|
Hi @IAlibay , thank you for the detailed feedback to answer your question. Yes, I did use an LLM assist with this contribution. My intention was to use it to help me understand the analysis structure and generate this initial refacing logic as I'm new to the code base. The errors you pointed out, especially the code placement before the docstring and the incorrect run() implementation were oversights on my part in reviewing the generated suggestions. I apologise for submitting a pr that wasn't up to the standard and for wasting your time, I will close this pr now. I plan to take a step back and study the documentation properly, so I can understand the class architecture before attempting to contribute again. |
Fixes #4665
Changes made in this Pull Request:
HydrogenBondAutoCorrelto inherit fromAnalysisBaseinstead ofAnalysisFromFunction.run()method to accept standard arguments (start,stop,step,verbose) and utilizeself._slice_traj.client_HydrogenBondAutoCorrelfixture totestsuite/MDAnalysisTests/analysis/conftest.py(excluding multiprocessing).hbond_autocorrelfixture totestsuite/MDAnalysisTests/analysis/conftest.pyfor test instantiation.TestHydrogenBondAutoCorrelto use the new client fixture._slice_trajto raiseValueErrorfor unslicable trajectories.PR Checklist
package/CHANGELOGfile updated?package/AUTHORS? (I added that in another PR that is under review)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--5195.org.readthedocs.build/en/5195/