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 quasiharmonic job #1040

Merged
merged 1 commit into from
May 15, 2023
Merged

fix quasiharmonic job #1040

merged 1 commit into from
May 15, 2023

Conversation

jan-janssen
Copy link
Member

Closes #1039

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4951498909

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 67.759%

Totals Coverage Status
Change from base Build 4914673511: 0.01%
Covered Lines: 11208
Relevant Lines: 16541

💛 - Coveralls

@pmrv
Copy link
Contributor

pmrv commented May 11, 2023

Does it make sense for users to modify the axis/strains on the level of the quasi harmonic approximation job? If so, I'm ok with this solution.

I guess the awkward thing here is that the murnaghan job generator is used by two master jobs, but it must, by design, assume explicit details of the master job it is assigned. I wasn't aware we actually use the same generator for multiple masters that are not in a inheritance relationship. Is that the only case?

@jan-janssen
Copy link
Member Author

Does it make sense for users to modify the axis/strains on the level of the quasi harmonic approximation job? If so, I'm ok with this solution.

It is mainly for compatibility - if I do not have the axis it fails with a very similar error.

Is that the only case?

Given the small number of GenericMasters we have I think this is fair to assume. Still this is where an IDE can be helpful to show places where a given code is used.

@pmrv
Copy link
Contributor

pmrv commented May 13, 2023

Does it make sense for users to modify the axis/strains on the level of the quasi harmonic approximation job? If so, I'm ok with this solution.

It is mainly for compatibility - if I do not have the axis it fails with a very similar error.

I see that, but the alternative fix would be to change the murnaghan generator to handle None or non-existing entries in its master's input. That would leave the user input of the quasi-harmonic job unchanged. So it comes down to which interface we want and that's why I asked this.

@jan-janssen
Copy link
Member Author

I see that, but the alternative fix would be to change the murnaghan generator to handle None or non-existing entries in its master's input. That would leave the user input of the quasi-harmonic job unchanged. So it comes down to which interface we want and that's why I asked this.

The Quasi-harmonic Job requires an Murnaghan job as input to calculate the free energies as discussed in https://pyiron.org/phasediagram-workshop-2020/Exercise4.html . So consequently it should support the same functionality. I am not sure if we need the functionality for the Murnaghan job as I primarily use uniform displacements, but I am open to allow non-uniform displacements. Still if we do, then we should also make sure that the features remain consistent over the whole pyiron project. Consequently, the alternative to me would be reverting #816

@pmrv pmrv self-requested a review May 15, 2023 07:20
@jan-janssen jan-janssen merged commit fa3e2fa into main May 15, 2023
@delete-merged-branch delete-merged-branch bot deleted the fix_quasi_harmonic branch May 15, 2023 13:07
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.

QuasiHarmonicJob is broken
3 participants