Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix read and write from pmb.df #116

Merged
merged 8 commits into from
Mar 4, 2025

Conversation

pm-blanco
Copy link
Collaborator

@pm-blanco pm-blanco commented Feb 18, 2025

Solves #102 and #114 finally.

Changed

Fixed

  • Writing and reading pmb.df from file does no longer change the variable type from int to float when there are empty cells in the column.

I had to do quite a bit of refactoring to make the code work with pd.NA because it behaves differently than np.nan, but I checked that all functional tests still run.

@paobtorres @1234somesh this PR solves the issues you raised!

@pm-blanco pm-blanco added bug Something isn't working code quality labels Feb 18, 2025
@pm-blanco pm-blanco added this to the pyMBE 1.0.0 milestone Feb 18, 2025
@pm-blanco pm-blanco requested a review from jngrad February 18, 2025 08:52
@pm-blanco pm-blanco self-assigned this Feb 18, 2025
@jngrad
Copy link
Member

jngrad commented Feb 18, 2025

To solve the bookkeeping of ._bond_id I had to hack a bit the espresso bond object because otherwise it was hard to backtrack it.

You may want to discuss it with @RudolfWeeber, he is planning to rewrite the bond id management in the next release of ESPResSo. It would be best if we could update pyMBE at the same time we update the bond id management in ESPResSo.

@pm-blanco
Copy link
Collaborator Author

You may want to discuss it with @RudolfWeeber, he is planning to rewrite the bond id management in the next release of ESPResSo. It would be best if we could update pyMBE at the same time we update the bond id management in ESPResSo.

Should we discuss it today during the ESPResSo coding day or better in our meeting scheduled next week?

In principle the issue with ._bond_id is not breaking the functionalities of pyMBE at the moment (at least that we are aware of) so I could revert the patch and wait for the next ESPResSo release to fix it. We could also leave the patch and adapt it to the new framework once the new version of ESPResSo is released, it is actually not too many lines of code that are involved.

@pm-blanco
Copy link
Collaborator Author

pm-blanco commented Feb 20, 2025

@jngrad I tested the handling of the _bond_id private attribute of bond objects in espresso and it seems to be robust. Bonds seem to retain the same _bond_id, even if you delete one bond and afterwards you save and load a checkpoint. See a minimal working example below.

Should then keep this PR as it is, or you think is better that we remove support for tracking of _bond_id in pyMBE for now?

import espressomd
from espressomd import interactions
from espressomd import checkpointing

restart=True
# Set-up the checkpoint
checkpoint = checkpointing.Checkpoint(checkpoint_id=f"mycheckpoint", 
                                      checkpoint_path=".")

if restart:
        checkpoint.load()
    
else:
        Box_L = 7.5*pmb.units.nm
        espresso_system = espressomd.System(box_l = [Box_L.to('reduced_length').magnitude]*3)
        espresso_system.time_step=0.1
        checkpoint.register("espresso_system")
        bond    = interactions.HarmonicBond(k   = 0,
                                                r_0 = 1)

        bond1    = interactions.HarmonicBond(k   = 1,
                                                r_0 = 1)
        bond2    = interactions.HarmonicBond(k   = 2,
                                                r_0 = 1)


        bond_id = espresso_system.bonded_inter.add(bond)
        bond_id1 = espresso_system.bonded_inter.add(bond1)
        bond_id2 = espresso_system.bonded_inter.add(bond2)

        for item in espresso_system.bonded_inter.items():
                print(item)

        espresso_system.bonded_inter.remove(bond_id1)
        checkpoint.save()

for item in espresso_system.bonded_inter.items():
     print(item)

Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

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

Besides the requested change, LGTM. The bond id business will need to be revisited when we refactor the bond storage mechanism in ESPResSo, but that project is months away.

jngrad
jngrad previously approved these changes Mar 4, 2025
@pm-blanco pm-blanco merged commit dbdc15a into pyMBE-dev:main Mar 4, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants