Fix guess_TopologyAttrs for empty Elements and Atomtypes#5416
Fix guess_TopologyAttrs for empty Elements and Atomtypes#5416AnimeshPriyanshu wants to merge 1 commit into
Conversation
|
AnimeshPriyanshu
force-pushed
the
fix-5414-missing-value-label
branch
from
2ee98ed to
4c1cd4b
Compare
There was a problem hiding this comment.
Have you looked deeper into the guessers? Is there perhaps a reason why Elements does not have a value?
There's another failing test test_wrong_elements_warnings whose outcome your patch changes. You need to understand if that should or should not happen.
You are also renaming and changing an existing test. You should not be doing this lightly as these tests may check expected behavior. At a minimum you have to explain why you are making these changes.
I understand that the poster on issue #5414 suggested the minimal fix but to be honest, this looked like a LLM-suggestion to me. That doesn't mean it's necessarily wrong but it may not have the full picture.
Basically, can you explain in your own words why your fix is the right solution to the problem?
| atol=0, | ||
| ) | ||
|
|
||
| def test_partial_guess_attr_with_unknown_no_value_label(self): |
There was a problem hiding this comment.
The fact that there's a test that checks for the behavior that you're changing should at least give pause for thought.
Why are you changing the name and the outcome for this test?
| assert_equal(u.atoms.types, ["", "", "", ""]) | ||
| assert_equal(u.atoms.types, ["C", "H", "H", "O"]) | ||
|
|
||
| def test_partial_guess_elements_with_missing_value_label(self): |
There was a problem hiding this comment.
Adding this test that checks for your functionality is ok.
There was a problem hiding this comment.
There are a lot of formatting changes in the file that make the diff hard to read. Why did you change the formatting?
2 participants
Fixes #5414
Changes made in this Pull Request
missing_value_label = ""toElementsandAtomtypesso empty string values are recognized as missing during topology attribute guessing.Atomtypespartial guessing test to reflect the new behavior.LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: no
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS? (If it is not, add it!)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.