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

ElasticMaker shouldn't use submaker as default factory by design #845

Open
chiang-yuan opened this issue May 12, 2024 · 3 comments
Open

Comments

@chiang-yuan
Copy link
Contributor

ElasticMaker shouldn't use submaker as default factory otherwise chgnet will be required to be installed as dependency

bulk_relax_maker: ForceFieldRelaxMaker | None = field(
default_factory=lambda: CHGNetRelaxMaker(
relax_cell=True, relax_kwargs={"fmax": 0.00001}
)
)
elastic_relax_maker: ForceFieldRelaxMaker | None = field(
default_factory=lambda: CHGNetRelaxMaker(
relax_cell=False, relax_kwargs={"fmax": 0.00001}
)

@utf
Copy link
Member

utf commented May 13, 2024

So currently, this doesn't actually require CHGNet to be installed I don't think. I.e., you can import this and overwrite those makers to get a MACE or M3GNet elastic constant workflow. However, it could make sense to create several versions of the workflow, one for each universal force field?

One thing I realised looking at this is that we can remove the prev_calc_dir_argname function from the BaseElasticMaker and subclasses since this has now been standardised across all codes.

@JaGeo
Copy link
Member

JaGeo commented May 13, 2024

So currently, this doesn't actually require CHGNet to be installed I don't think. I.e., you can import this and overwrite those makers to get a MACE or M3GNet elastic constant workflow. However, it could make sense to create several versions of the workflow, one for each universal force field?

One thing I realised looking at this is that we can remove the prev_calc_dir_argname function from the BaseElasticMaker and subclasses since this has now been standardised across all codes.

@utf also in the Abinit workflows?

@chiang-yuan
Copy link
Contributor Author

Yes @utf you are definitely right - one could always replace default factory with the relaxmaker they like to use. However, I think this design is a bit counterintuitive and one might just want to set the forcefield calculator name and the code switch the input generator accordingly like what we did for MDMaker

Not a serious issue so maybe I shouldn't call it bug at the first place (I think it is the template..)

@chiang-yuan chiang-yuan changed the title BUG: ElasticMaker shouldn't use submaker as default factory ElasticMaker shouldn't use submaker as default factory by design May 15, 2024
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

No branches or pull requests

3 participants