fix: Add missing comma in LHEInit fieldnames

[matthewfeickert]

Fixes the bug in Issue #281

Not sure we have a good test at the moment though.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.87%. Comparing base (7519967) to head (62e326a).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #282   +/-   ##
=======================================
  Coverage   91.87%   91.87%           
=======================================
  Files           2        2           
  Lines         320      320           
  Branches       64       64           
=======================================
  Hits          294      294           
  Misses         17       17           
  Partials        9        9           

Flag	Coverage Δ
unittests-3.10	91.87% <100.00%> (ø)
unittests-3.11	91.87% <100.00%> (ø)
unittests-3.12	91.87% <100.00%> (ø)
unittests-3.13	91.87% <100.00%> (ø)
unittests-3.8	91.87% <100.00%> (ø)
unittests-3.9	91.87% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[APN-Pucky]

Here's what we could do. But it seems a bit unnecessary unless we want to restrict the access to the predefined fields (breaking previous "free" behavior). And just defining the list of fields in a second place (tests) seems a bit wasted/stupid as a test. I must say I don't see the point in having the variable fieldnames, unless we loop over it, but maybe there are some builtin dict functions that use it.

class CustomDict(dict):
    fieldnames = ['name', 'age', 'email']

    def __setitem__(self, key, value):
        if key not in self.fieldnames:
            raise KeyError(f"Invalid key: {key}. Allowed keys are {self.fieldnames}")
        super().__setitem__(key, value)

import pytest
from your_module import CustomDict  # Import your class

def test_valid_keys():
    data = CustomDict()
    data['name'] = 'Alice'  # Should work
    data['age'] = 30        # Should work

def test_invalid_key():
    data = CustomDict()
    with pytest.raises(KeyError, match="Invalid key: address"):
        data['address'] = '123 Street'  # Should fail

[eduardo-rodrigues]

Hello. I tend to agree that a test seems a bit of an overkill. On the other hand I'm sad the bug did not get caught.

[matthewfeickert]

I must say I don't see the point in having the variable fieldnames, unless we loop over it, but maybe there are some builtin dict functions that use it.

I tend to agree that a test seems a bit of an overkill.

Yeah, I agree with you both that adding a test as is seems like a lot of superfluous work. I think we can skip that for this PR and maybe just call this good?

Though I agree with @APN-Pucky that I don't off the top of my head remember why LHEInit has fieldnames

pylhe/src/pylhe/__init__.py

Line 251 in 7519967

fieldnames = ["initInfo", "procInfo" "weightgroup", "LHEVersion"]

as LHEInit doesn't have a fromstring like LHEInitInfo does

pylhe/src/pylhe/__init__.py

Lines 340 to 342 in 7519967

	@classmethod
	def fromstring(cls, string):
	return cls(**dict(zip(cls.fieldnames, map(float, string.split()))))

and so fieldnames is never used in LHEInit (which is why we've never seen this as a bug).

Ah I guess looking a bit more the fieldnames were added in PR #233 when large components of what used to be LHEInit got moved into LHEInitInfo.

@APN-Pucky @eduardo-rodrigues If we don't explicitly use

pylhe/src/pylhe/__init__.py

Line 251 in 7519967

fieldnames = ["initInfo", "procInfo" "weightgroup", "LHEVersion"]

then should we keep them at all? (I am going through this quickly while task switching, so I might be missing the obvious.)

[matthewfeickert]

Suggested change

fieldnames = ["initInfo", "procInfo", "weightgroup", "LHEVersion"]

If we don't explicitly use

pylhe/src/pylhe/__init__.py

Line 251 in 7519967

fieldnames = ["initInfo", "procInfo" "weightgroup", "LHEVersion"]

then should we keep them at all?

Given that this line was never getting used (else we would have seen this non-list list break) I think we can just remove this.

[eduardo-rodrigues]

I had a look at the module. Most other classes have a fromstring method, which uses fieldnames. This one LHEInit doesn't. I'm no expert to understand why. Would it be useful to add such a method?

In any case, one thing the variable fieldnames is good for is documentation. Right now the docstring is minimalistic and maybe that's worth a revamp as well?

[APN-Pucky]

Most other classes have a fromstring method, which uses fieldnames. This one LHEInit doesn't. I'm no expert to understand why. Would it be useful to add such a method?

I would be in favor of adding a fromstring there too. It sort of exists as part of read_lhe_init(file_or_string) function, but that can then be mapped to use the new fromstring.

The test on the fieldnames values would then just be if tolhe(fromstring(...)) and fromstring(tolhe(...)) are consistent.

[APN-Pucky]

I created a PR to add fromstring #283, but just like the read_lhe_init it does not use fieldnames and I don't see the point in forcing it to use it now.