Skip to content

Conversation

@steinmig
Copy link
Contributor

  • some small typing corrections and cleanups

Copy link
Contributor

@HojeChun HojeChun left a comment

Choose a reason for hiding this comment

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

If I understand correctly, I like your idea of unifying various calculators. But, I have some concerns you might want to consider.
Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure why you made changes for this. if you need load_foundations_path function as class methods, I guess you keep the original function and make load_foundations_path and make something like.

@classmethod
def load_foundations_path (cls, model, map_location, default_dtype):
        mace_model_path = get_mace_mp_model_path(model)
        return cls.load_foundations_path(mace_model_path, map_location, default_dtype)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't quite understand what you mean. As far as I understand your comment, this is what I did

Copy link
Contributor

Choose a reason for hiding this comment

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

i mean get_mace_mp_model_path is basically load_foundations_path. If your intention was to have a classmethod of getting model_path I would decouple the functions as

@classmethod
def load_foundations_path (cls, model, map_location, default_dtype):
        mace_model_path = get_mace_mp_model_path(model)
        return mace_model_path

Sorry for a wild example above

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this idea, and actually I have implemented this kind of thing for my vssr calculator.

raise RuntimeError("Batch is missing 'nxyz' key.")
pbc = batch.get("pbc")
if pbc is not None:
pbc = np.array(pbc, dtype=bool)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is pbc going to be size (3,)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly. It's basically the pbc member of ase.Atoms

Copy link
Contributor

@xiaochendu xiaochendu left a comment

Choose a reason for hiding this comment

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

Looks good to me. I only have a single comment regarding the size of pbc in AsePotential

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.

3 participants