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

VASP IO copy() methods #3602

Merged
merged 15 commits into from
Feb 8, 2024
Merged

VASP IO copy() methods #3602

merged 15 commits into from
Feb 8, 2024

Conversation

janosh
Copy link
Member

@janosh janosh commented Feb 5, 2024

as pointed out by @esoteric-ephemera, calling .copy() on VASP IO classes like Incar and VaspInput returns a dict rather than a new instance of the class as most users would expect. this PR adds explicit copy() methods to minimize user surprise.

@esoteric-ephemera that mixin snippet i sent doesn't work as the __init__ signatures of IO classes are too different. for the record, we considered defining

class CopyableMixin:
    def copy(self):
        return type(self)(self)

and making all Vasp IO classes inherit from it

@janosh janosh added enhancement A new feature or improvement to an existing one io Input/output functionality vasp Vienna Ab initio Simulation Package labels Feb 5, 2024
@esoteric-ephemera
Copy link
Contributor

Thanks @janosh! Question for @shyuep: @janosh and I were discussing if it makes more sense to define the self.copy method for MSONable - do you think it makes more sense to define the copy method for specific classes that inherit from MSONable, or for MSONable itself?

@shyuep
Copy link
Member

shyuep commented Feb 5, 2024

Depends on how general the copy method is. If it is generic and uses the MSONAble capabilities to do the copy, then it makes sense.

@shyuep
Copy link
Member

shyuep commented Feb 5, 2024

Just an addendum that when I say "makes sense", it means it makes sense for any MSONable class, not just VASP io related stuff.

Copy link
Member Author

@janosh janosh left a comment

Choose a reason for hiding this comment

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

might also make sense to turn all VASP IO classes into @dataclasses so we get __eq__ for free and copy with minimal variation

@janosh janosh merged commit c910356 into master Feb 8, 2024
22 checks passed
@janosh janosh deleted the vasp-io-copy-methods branch February 8, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement to an existing one io Input/output functionality vasp Vienna Ab initio Simulation Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants