shirtsgroup / physical_validation

Physical validation of molecular simulations
https://physical-validation.readthedocs.io
MIT License
55 stars 19 forks source link

Compatibility with pymbar 4 #216

Closed ptmerz closed 1 year ago

ptmerz commented 2 years ago

New release of pymbar breaks compatibility. We should

mrshirts commented 2 years ago

Yes, I should have checked this one explicitly! Pinning pymbar < 4.0.0 will fix the current issues. I can go in over the next few weeks when I have a chance and do a PR with the proposed changes to 4.0.

dora1300 commented 1 year ago

Will there be an update to this soon? I'm still facing the erroneous call errors popping up.

ptmerz commented 1 year ago

I'll have a look into this (and should have done this a while ago).

As a side note, I think this reinforces the importance of moving away from pymbar (see also #48). I'm not too happy having a dependency on a package that introduces API-breaking changes without previous deprecation. Actually, scratch this. Going over our uses of pymbar, I noticed that we are also using pymbar's BAR functionality, so we will definitely keep a dependency on pymbar for the foreseeable future.

ptmerz commented 1 year ago

I proposed a fix at #220.

I opted to keep physical_validation compatible to pymbar 3 and 4. My reasoning is that different packages will handle the change of API of pymbar at different paces, and I would like to avoid creating conflicts for users. Here's an example to illustrate this point: alchemlyb has, at least for now, opted to require pymbar <4. If we would update all our API calls to the new version, and require pymbar >= 4, we would create conflicts for users that would want to install physical_validation and alchemlyb in the same environment.

I'm happy to hear your thoughts, @mrshirts and @mattwthompson.

mattwthompson commented 1 year ago

Supporting multiple APIs of an upstream library is often difficult (if possible) to implement and is an easy way to create future maintainability headaches, so I generally prefer to avoid it. In this situation, though, I think it would be a significant barrier to adoption and continued usage to dictate which version of pymbar is used by another tool. And, given that you've already done the implementation, I can't see a reason to say we should only support 4+ now.

The only possible downside here, I think, is implying support for pymbar 3 in the indefinite future. Hopefully in a few years people will have updated their code to new versions of things, but maybe they won't (🙃). Setting out a policy of "support for this will end in x years" (where is is some number maybe around 2-3) might be useful for justifying dropping this support in the future.

If others feel positive about this change as well, we should merge #220 and make a release sometime soon.

mrshirts commented 1 year ago

Yeah, it's not ideal that we moved the API in pymbar 4 without deprecation warnings, but we had a bunch of stuff that had been just languishing for four years, and we weren't sure when there would be another chance to get stuff out the door. @Lnaden and I were the only people coding (and it was mostly me).

The next alchemlyb release will only support pymbar 4.0. So I think sunsetting pymbar 3 after 2-ish years is very reasonable. pymbar 3 will not be developed except for maybe bugfixes, and even then we will try to push people to 4.0 if possible.

ptmerz commented 1 year ago

I definitely share some of your reservations @mattwthompson, but I felt that in this case, the implementation is straightforward enough, and the downsides big enough, to warrant supporting this for a while.

I think what we can do for physical_validation is to deprecate pymbar-3 support in a little while (and add a warning in the except block of the try/except constructs), and then phase it out.