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

Make generic energy_pot of Vasp configurable #1227

Closed
wants to merge 2 commits into from
Closed

Make generic energy_pot of Vasp configurable #1227

wants to merge 2 commits into from

Conversation

pmrv
Copy link
Contributor

@pmrv pmrv commented Nov 29, 2023

Vasp outputs three energies, smeared internal energy, free energy and smeared internal extrapolated to zero smearing.
This change allows to pick which of them is set as

output/generic/energy_pot

of the job.

Current defaults are smeared internal extrapolated to zero smearing when vasprun.xml can be read and free energy when OUTCAR is read.

@pmrv pmrv added bug Something isn't working enhancement New feature or request labels Nov 29, 2023
Vasp outputs three energies, smeared internal energy, free energy and
smeared internal extrapolated to zero smearing.
This change allows to pick which of them is set as

output/generic/energy_pot

of the job.

Current defaults are smeared internal extrapolated to zero smearing when
vasprun.xml can be read and free energy when OUTCAR is read.
@coveralls
Copy link

coveralls commented Nov 29, 2023

Pull Request Test Coverage Report for Build 7036454874

  • 26 of 40 (65.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 68.619%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyiron_atomistics/vasp/base.py 26 40 65.0%
Totals Coverage Status
Change from base Build 7036095399: -0.02%
Covered Lines: 10800
Relevant Lines: 15739

💛 - Coveralls

@pmrv
Copy link
Contributor Author

pmrv commented Nov 29, 2023

This also sidesteps a bug in the 5.4.4 xml output.

Ideally, I'd like to make ENERGY_FREE the default, because otherwise the "generic" forces are not consistent with the "generic" energies and that just doesn't make sense. For now I've kept the previous behavior though.

I'm not married to the name energy_kind because it's not very self explanatory, but couldn't think of anything right now.

@samwaseda
Copy link
Member

Hmmmmmmmmmmmm, I find it super dangerous to be able to change the default. Actually in a meeting long long time ago we kind of agreed to use the free energy for energy_pot. But anyway I have the feeling that this requires a bit more than just a review.

@pmrv
Copy link
Contributor Author

pmrv commented Nov 29, 2023

I definitely need a way for output/generic/energy_pot to be the free energy. I'm ok if that's the only way, but it would break behavior so that's how I ended up this way.

@samwaseda
Copy link
Member

ok then I would rather support the idea of changing it to the free energy. Anything else would be for me nonsense because all the other derivative properties (forces, pressures etc.) come from the free energy.

@samwaseda
Copy link
Member

Also: if we decide to make it optional, I think we need a better concept to choose output values. That's something @jan-janssen and I talked about yesterday, and from the performance point of view and also in the context of workflow I fully support it, but I would not just want to create a new entry in the input and say this is how it works from now on.

@pmrv
Copy link
Contributor Author

pmrv commented Nov 30, 2023

Is there any other place that makes sense to configure it, though? If output/parsing/etc. behavior can be changed, I would want it on the job level, because otherwise I'll need to keep global state in my head as I work between different projects. If it is to live on a job, input seems the correct location? One can think about a more "generic" input where options like this live, but a) we don't have one yet and b) these parser options would likely by very code specific.

@pmrv
Copy link
Contributor Author

pmrv commented Nov 30, 2023

Either way, the priority for me is to change the output to the free energy. I can simplify this and we discuss the optional stuff later.

@samwaseda
Copy link
Member

Is there any other place that makes sense to configure it, though?

No there isn't. For interactive LAMMPS we have something similar here though, which is not really to be touched, but as we all know, this kind of stuff is not to be touched, until people start touching it, and in the end we have the same functionality with different standards everywhere.

If output/parsing/etc. behavior can be changed, I would want it on the job level, because otherwise I'll need to keep global state in my head as I work between different projects. If it is to live on a job, input seems the correct location?

No I don't think it's the "correct" location, but it's the only one "not impossible" location, because our input currently does not talk about output (except for the phonopy job, which you and I hate). If we include the output info in the input, then I think we need a clear interface with a well defined structure.

One can think about a more "generic" input where options like this live, but a) we don't have one yet and b) these parser options would likely by very code specific.

Yes, I agree, and just like the interactive class that I mentioned above, it's not necessarily code specific.

@pmrv
Copy link
Contributor Author

pmrv commented Dec 4, 2023

  • bump vasp version number in job and parse the correct energy
  • add a maintenance function that
    1. changes the energy_pot to energy_free
    2. check whether there are any master jobs
  • close this

@pmrv pmrv closed this Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants