WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Conversation

@tanii1125
Copy link

@tanii1125 tanii1125 commented Dec 9, 2025

This PR implements enhancement proposed in issue #5170

Summary

EinsteinMSD currently lowercases the msd_type string but does not:

  • strip whitespace
  • validate the type
  • handle mixed-case safely

What This PR Does?

  1. Add .strip() before .lower() to handle inputs like " xy ".
  2. Add type validation (raise TypeError when msd_type is not a string).
  3. Add new tests to cover:
    • whitespace (" xy ")
    • uppercase/mixed case (" Xz ")
    • non-string inputs

All tests pass locally


📚 Documentation preview 📚: https://mdanalysis--5173.org.readthedocs.build/en/5173/

@codecov
Copy link

codecov bot commented Dec 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.72%. Comparing base (bbcef1b) to head (ced5ece).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #5173   +/-   ##
========================================
  Coverage    92.72%   92.72%           
========================================
  Files          180      180           
  Lines        22472    22477    +5     
  Branches      3188     3189    +1     
========================================
+ Hits         20837    20842    +5     
  Misses        1177     1177           
  Partials       458      458           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines 385 to 390
# Validate type first
if not isinstance(self.msd_type, str):
raise TypeError("msd_type must be a string")

# strip whitespace + lowercase
self.msd_type = self.msd_type.strip().lower()
Copy link
Author

@tanii1125 tanii1125 Dec 13, 2025

Choose a reason for hiding this comment

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

Added these lines in msd.py , they do -

  1. Explicitly checks that msd_type is a string to avoid unexpected AttributeErrors.
  2. Uses .strip().lower() to handle inputs with leading/trailing whitespace
    (e.g. " xy ", " Xz "), while preserving existing case-insensitive behavior.

Comment on lines 126 to 139
def test_msd_type_whitespace(self, u, SELECTION):
m = MSD(u, SELECTION, msd_type=" xy ", fft=False)
assert m.dim_fac == 2
assert m._dim == [0, 1]

def test_msd_type_uppercase(self, u, SELECTION):
m = MSD(u, SELECTION, msd_type=" Xz ", fft=False)
assert m.dim_fac == 2
assert m._dim == [0, 2]

def test_msd_type_nonstring(self, u, SELECTION):
with pytest.raises(TypeError):
MSD(u, SELECTION, msd_type=123, fft=False)

Copy link
Author

@tanii1125 tanii1125 Dec 13, 2025

Choose a reason for hiding this comment

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

Added tests covering msd_type input handling in EinsteinMSD:

  • whitespace handling (e.g. " xy ")
  • case-insensitive parsing (existing behavior)
  • non-string inputs raising TypeError

@marinegor marinegor self-requested a review December 23, 2025 17:29
@Dreamstick9
Copy link

Hey! Just checking out the changes.

Since this ecosystem relies heavily on NumPy, users might be passing msd_type as a numpy.str_ (like if they read their config from an array).
Screenshot 2025-12-24 at 9 39 02 AM
This check would strictly raise a TypeError for them, even though it likely worked fine before via duck typing.

It might be safer to either check against (str, np.str_) or just let the .strip().lower() call handle it implicitly so we don't break those workflows.

Also, minor edge case: if the input is just whitespace (e.g. " "), strip() will result in an empty string "". It might be worth raising a ValueError there so it doesn't fail silently downstream?

Nice cleanup on the parsing logic though!

@tanii1125
Copy link
Author

@Dreamstick9 thank you for pointing this out !

Since the use of .strip() is in question, I’ll update this PR accordingly:

  • keep .lower() while avoiding breaking string-like inputs (e.g. numpy.str_)
  • add an explicit check to raise a clear error for empty input

@Dreamstick9
Copy link

One more thing I noticed in test_msd.py:

test_msd_type_whitespace_around_valid_value expects a ValueError for " xy ".

Since the PR goal is to support whitespace, shouldn't this test expect success instead of an error?

Currently, this test will likely fail because the code raises a KeyError (due to the unstripped string) rather than the expected ValueError.

@tanii1125
Copy link
Author

This is intentional. As discussed in #5170, we decided not to silently normalize whitespace. Inputs like " xy " are treated as invalid, and the test documents that behavior explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants