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

Castep ase323 update #330

Merged
merged 9 commits into from
Aug 16, 2024
Merged

Castep ase323 update #330

merged 9 commits into from
Aug 16, 2024

Conversation

gelzinyte
Copy link
Contributor

No description provided.

wfl/calculators/castep.py Outdated Show resolved Hide resolved
@gelzinyte
Copy link
Contributor Author

The ASE v3.22 and v3.23 Castep versions aren't easy to reconcile like it was in the other profile-based calculators. Mainly because they made it more consistent with the rest of the ASE calculators. I've left comments in the two relevant places.

I think the most straightforward solution is to enforce ASE v3.23 for the castep implementation, while we're still allowing (if not testing) ASE v3.22 for the other calculators.

To make both acceptable, we would need to check the version and implement different choices in the two places I've highlighted. For example, call self.calculate(atoms) for v3.22 and self.calculate(atoms, properties, allow_calculation) for v2.23; check that self.get_property() returns None in v3.23 and check if self._energy_totalisNone` in v3.22. I think that's messy and we should move towards only allowing 3.23 in the requirements eventually.

@bernstei thoughts?

There are no major changes otherwise and the tests pass with Castep23 locally, so this PR is ready to merge.

@bernstei
Copy link
Contributor

bernstei commented Aug 7, 2024

I want to do a brief review, but I think it'll be fine.

we should move towards only allowing 3.23 in the requirements eventually.

My preference would be to just force the move to 3.23 now, because I'd rather keep our code simpler. And 3.23 was so delayed, it has tons of updates/fixes. But I do admit that the changes were so substantial that I can see people wanting to stick for 3.22 for a while to avoid breaking existing scripts.

Is there any way for us to gauge whether any users actually want pre-3.23 support?

@bernstei
Copy link
Contributor

bernstei commented Aug 7, 2024

See the one brief question/comment above, otherwise let's decide about the 3.22 support, and then merge.

@gelzinyte
Copy link
Contributor Author

My preference would be to just force the move to 3.23 now, because I'd rather keep our code simpler. And 3.23 was so delayed, it has tons of updates/fixes. But I do admit that the changes were so substantial that I can see people wanting to stick for 3.22 for a while to avoid breaking existing scripts.

Is there any way for us to gauge whether any users actually want pre-3.23 support?

In that case, I would like to update the requirement to 3.23. I think that for 3.22 compatibility, we could add a note in the README, pointing to the latest wfl version before this update/next tag. I'll add the relevant changes (in this PR?) if that sounds good.

@bernstei
Copy link
Contributor

Sounds good to me.

@gelzinyte
Copy link
Contributor Author

gelzinyte commented Aug 16, 2024

Maybe let's first merge this and orca PRs and bump/fix the version & pip in a separate PR (I'm a bit confused about how it works, so would like to not mix it in here).

Otherwise, this is ready to merge from my side, feel free to do so.

@gelzinyte gelzinyte mentioned this pull request Aug 16, 2024
3 tasks
@bernstei bernstei merged commit 4db7936 into main Aug 16, 2024
4 of 5 checks passed
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.

2 participants