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 to stop convergence checks for interactive jobs! #222

Merged
merged 6 commits into from
Jun 7, 2021

Conversation

sudarsan-surendralal
Copy link
Member

A temporary fix! Please feel free to modify this if necessary

@sudarsan-surendralal sudarsan-surendralal added the bug Something isn't working label Jun 1, 2021
@sudarsan-surendralal sudarsan-surendralal linked an issue Jun 1, 2021 that may be closed by this pull request
@coveralls
Copy link

coveralls commented Jun 1, 2021

Pull Request Test Coverage Report for Build 914266187

  • 3 of 4 (75.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 67.643%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyiron_atomistics/vasp/interactive.py 3 4 75.0%
Totals Coverage Status
Change from base Build 909017515: 0.002%
Covered Lines: 10666
Relevant Lines: 15768

💛 - Coveralls

@pmrv
Copy link
Contributor

pmrv commented Jun 1, 2021

We could also put this into GenericInteractive, no?

@liamhuber
Copy link
Member

We could also put this into GenericInteractive, no?

If I understand, then we rely entirely on the MRO and if anyone switched class VaspInteractive(VaspBase, GenericInteractive) to class VaspInteractive(GenericInteractive, VaspBase) we would break the functionality because of all the funky convergence check jazz in VaspBase. (Or it starts backwards and we would need to do this now? Honestly I can never remember the order of resolution.) I think it's unlikely that someone would go around messing with the inheritance structure without knowing what they're doing/having a good reason, but it still seems like it adds unnecessary fragility.

Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

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

In the long run we need to adjust the inheritance tree so this sort of awkwardness is not necessary, but in the short run I don't see any problems or have any better suggestions!

@pmrv
Copy link
Contributor

pmrv commented Jun 7, 2021

We could also put this into GenericInteractive, no?

If I understand, then we rely entirely on the MRO and if anyone switched class VaspInteractive(VaspBase, GenericInteractive) to class VaspInteractive(GenericInteractive, VaspBase) we would break the functionality because of all the funky convergence check jazz in VaspBase. (Or it starts backwards and we would need to do this now? Honestly I can never remember the order of resolution.) I think it's unlikely that someone would go around messing with the inheritance structure without knowing what they're doing/having a good reason, but it still seems like it adds unnecessary fragility.

Works for me, just wanted to raise the point.

@liamhuber
Copy link
Member

Works for me, just wanted to raise the point.

It was a good one. I'm 100% behind the principle of pushing the method as far upstream as it still makes sense (and it really makes sense in GenericInteractive, it's just that here the streams get too crossed 😝

@sudarsan-surendralal sudarsan-surendralal merged commit 06d54ac into master Jun 7, 2021
@delete-merged-branch delete-merged-branch bot deleted the vasp_int_conv_check branch June 7, 2021 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running VASP interactively throws a BrokenPipeError
4 participants