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

Review/update to work with propka >= 3.3.0 #20

Open
orbeckst opened this issue Jul 4, 2020 · 12 comments
Open

Review/update to work with propka >= 3.3.0 #20

orbeckst opened this issue Jul 4, 2020 · 12 comments

Comments

@orbeckst
Copy link
Member

orbeckst commented Jul 4, 2020

The new propka 3.2.0 release breaks the internal API in parts. To check

  • run.single() still works
  • getting the information from the molecular container still works
  • increase minimum propka version in setup.py
@IAlibay
Copy link
Collaborator

IAlibay commented Jul 4, 2020

So I've been playing with getting 3.2 to work (in light of #19 and a possible switch to using AnalysisBase).
Unfortunately it looks like quite a few things have changed such that backwards compatibility won't work (one particular case is that using 3.2's propka.input.read_parameter_file doesn't work with NamedStream, and well the fact that it seems that the only way to get things working is to use propka.input).

Since both 3.1 and 3.2 can now be pip installed, I would suggest the following (I'm willing to do the leg work here especially as it would be useful for some work I'm involved with):

  1. Set up Travis and add a basic regression test.
  2. Switch over to using AnalysisBase (that's easy enough so technically we don't need to do part 1, but having CI in place makes things a lot nicer).
  3. Making a final 3.1 release (at this point assuming we mostly rely on things in MDA that are API stable there shouldn't be any future changes).
  4. Make the necessary changes in the code to use 3.2 and, if we can't ensure backwards compatibility, make a 3.2-only release.

What do you think?

@orbeckst
Copy link
Member Author

orbeckst commented Jul 4, 2020

Unfortunately it looks like quite a few things have changed such that backwards compatibility won't work (one particular case is that using 3.2's propka.input.read_parameter_file doesn't work with NamedStream, and well the fact that it seems that the only way to get things working is to use propka.input).

This used to work earlier. Will fix it again in propka...

More later.

@orbeckst
Copy link
Member Author

orbeckst commented Jul 4, 2020

In general what you say sounds like an excellent plan.

@IAlibay
Copy link
Collaborator

IAlibay commented Jul 13, 2020

@orbeckst So I think we're at step 3 on the proposed schedule timeline. I don't know how you would feel about doing a release now-ish before moving on to propka 3.2 (we would need to drop python < 3.6 support, so it wouldn't be backwards compatible anymore)?

In terms of what is left to do in a final propka 3.1 release, there's #24. However, I doubt it will get fixed upstream in time for MDA 1.0.1. The fix is super trivial (maybe 20 lines?), but I can't seem to be able to find an official list of "PDB standard residues".

I need to get some trajectories analysed with propka 3.2 soon-ish (it's actually been my main motivation for working on this). So I'll probably try to get an initial test implementation done this weekend.

@orbeckst orbeckst mentioned this issue Jul 14, 2020
@orbeckst
Copy link
Member Author

Yes, I'll cut a 1.1.0 release #29 with what we have now and then it's onwards and upwards.

You can always do a PR against PROPKA and I can review it there if anything needs fixing in PROPKA (like the ability to use streams) – I am a bit snowed under with millions of things so I am not sure I can get to actual coding anytime soon.

@IAlibay
Copy link
Collaborator

IAlibay commented Jul 14, 2020

You can always do a PR against PROPKA and I can review it there if anything needs fixing in PROPKA (like the ability to use streams) – I am a bit snowed under with millions of things so I am not sure I can get to actual coding anytime soon.

Will do, I think I finally worked out what was causing the issue with streams in 3.2 and it's only ~ 2 typos it seems. There's a deeper issue with the way in which loadOptions works, but I'll just raise it on the Propka side of things.

@IAlibay
Copy link
Collaborator

IAlibay commented Jul 18, 2020

With run.single and read_molecule_file fixed in propka's dev branch switching the current code to use propka 3.2 is quite trivial. I think it's just a case of renaming resNumb to the 3.2 equivalent here:

self._columns = [g.atom.resNumb for g in groups]

I can put up a PR that works with the propka dev branch, but we'd have to wait until the next propka version (3.3?) is released for things to move forward again.

@orbeckst
Copy link
Member Author

We can install the propka git in the CI and just move forward. We'll make a 1.1.1 release when we have a corresponding propka release. (It's a patch-level increase because it's a bug-fix from our end.)

@orbeckst
Copy link
Member Author

I think we won't have a propkatraj version that can use proka 3.2.0 so we will have to explicit forbid it in setup.py.

@IAlibay
Copy link
Collaborator

IAlibay commented Feb 3, 2023

As discussed in #36 we should just try a quick PR to see if we can just do >3.3 since I think we fixed this upstream.

@orbeckst orbeckst changed the title Review/update to work with propka 3.2.0 Review/update to work with propka >= 3.3.0 Feb 10, 2023
@IAlibay
Copy link
Collaborator

IAlibay commented Feb 14, 2023

Mostly for the benefit of @orbeckst

@ianmkenney and I had a chat yesterday and it looks the way errors are generated for things like #10 has changed a lot since propka 3.3. Part of this seems to be due to a change in the underlying code where the issue itself doesn't happen anymore.

Our proposal was to go ahead with a 2.0 release and then do a quick 2.1 release when we have a bit more time to dig into if this yields a behaviour change? It breaks semver a little bit, but it's the easiest I could think of.

@orbeckst
Copy link
Member Author

Thanks for digging into the compatibility. I agree with your plan to get a quick working release out (possibly pinned). This will help with reproducibility. Then we can assess the changes. It's possible that PROPKA simply improved it's own capabilities. For the tests for "bad frames" we might have just taken examples from the wild that failed and then put in the skip.

@orbeckst orbeckst removed this from the Release 2.0.0 milestone Feb 14, 2023
@orbeckst orbeckst mentioned this issue Feb 14, 2023
8 tasks
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

No branches or pull requests

2 participants