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

Fixed a small issue with the erroneous attribute call on a Structure object in AMIN handler #297

Merged
merged 1 commit into from
Nov 5, 2023

Conversation

fyalcin
Copy link
Contributor

@fyalcin fyalcin commented Oct 19, 2023

Recently ran into this issue. Structure object doesn't have a structure attribute. This is now fixed.

@janosh
Copy link
Member

janosh commented Oct 19, 2023

Thanks @fyalcin! Would you be able to add a test for this?

@janosh janosh added handler Error handler fix Bug fix labels Oct 19, 2023
@codecov-commenter
Copy link

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (3ad487a) 65.78% compared to head (96aca09) 65.78%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #297   +/-   ##
=======================================
  Coverage   65.78%   65.78%           
=======================================
  Files          51       51           
  Lines        5632     5632           
=======================================
  Hits         3705     3705           
  Misses       1927     1927           
Files Coverage Δ
custodian/vasp/handlers.py 78.20% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Andrew-S-Rosen
Copy link
Member

Probably best to merge this fix and open an issue marking that we need to test that line.

@janosh
Copy link
Member

janosh commented Nov 5, 2023

I just noticed we currently have zero test coverage for NonConvergingErrorHandler. Sorry @fyalcin, didn't mean to imply you should write a new error handler test from scratch.

@Andrew-S-Rosen @matthewkuner Would either of you know where best to get reference files that match the NonConvergingErrorHandler.check conditions?

class NonConvergingErrorHandler(ErrorHandler):
    """
    Check if a run is hitting the maximum number of electronic steps at the
    last nionic_steps ionic steps (default=10). If so, change ALGO using a
    multi-step ladder scheme or kill the job.
    """

    def check(self):
        """Check for error."""
        vi = VaspInput.from_directory(".")
        n_elm = vi["INCAR"].get("NELM", 60)  # number of electronic steps
        try:
            oszicar = Oszicar(self.output_filename)
            elec_steps = oszicar.electronic_steps
            if len(elec_steps) > self.nionic_steps:
                return all(len(e) == n_elm for e in elec_steps[-(self.nionic_steps + 1) : -1])
        except Exception:
            pass
        return False

@janosh janosh merged commit 22d1956 into materialsproject:master Nov 5, 2023
2 checks passed
@matthewkuner
Copy link
Contributor

Personally, I would just manually modify the file to have extra electronic/ionic steps, or change the parameter for nionic_steps to be lower

@fyalcin
Copy link
Contributor Author

fyalcin commented Nov 7, 2023

I noticed the lack of error coverage for this particular handler but couldn't find the time to write one myself and decided to go without it. In hindsight, I should have mentioned this in the first message, sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix handler Error handler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants