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

Optionally return the dislocation core fit error. #247

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

imaliyov
Copy link
Contributor

Modified the fit_core_position() and fit_core_position_images() functions of the dislocation.py module.
return_error and return_errors flags added. If True, the functions will return the error of the scipy.optimize.minimize fit of the dislocation core position. This will require an additional cost_function() call. This probably can be avoided in the future if the error can be obtained directly from the minimize() function.

@jameskermode
Copy link
Member

@pgrigorev or @thomas-rocke would one of you be willing to take a look at this PR?

@pgrigorev
Copy link
Contributor

@jameskermode, we have already discussed this PR with Ivan, but I will have a closer look on the changes and let you know here.

@jameskermode
Copy link
Member

The failing tests are due to an ASE change, which could be fixed here or in a separate PR. I think ase.neb has moved to ase.mep.neb.

@jameskermode
Copy link
Member

We've also got some numpy 2 issues. I'll open an issue about that.

@pgrigorev
Copy link
Contributor

ah I see. Yes we should fix that. I guess the best way would be to check ase version on import to keep backward compatibility?

@pgrigorev
Copy link
Contributor

I think the tests failing tests have been fixed now, @imaliyov could you merge it to your branch?

@imaliyov
Copy link
Contributor Author

@pgrigorev thank you, I have merged it to my branch.

Copy link
Contributor

@pgrigorev pgrigorev left a comment

Choose a reason for hiding this comment

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

Everything looks good, I am sure it is safe to merge!

@pgrigorev
Copy link
Contributor

Sorry it took so long, but after having a look on the changes I think it is safe to merge. (the changes are really minor)

@jameskermode jameskermode merged commit 6809af7 into libAtoms:master Sep 4, 2024
27 checks passed
@jameskermode
Copy link
Member

Thanks!

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.

3 participants