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

scfix behaviour change #621

Merged
merged 8 commits into from
Oct 24, 2024
Merged

Conversation

csbrasnett
Copy link
Collaborator

@csbrasnett csbrasnett commented Oct 21, 2024

opening this as a PR for changing the default for -scfix.

Obviously atm this makes a bunch of tests fail. Is the best way to fix the test to:

  1. change the command being used as the input
  2. change the expected output of the test

or both?

Do we change the flag description too, to something like -[no]scfix, ie. when the flag is applied scfix gets turned off?

@csbrasnett csbrasnett changed the title Initial change in argument scfix behaviour change Oct 21, 2024
@pckroon
Copy link
Member

pckroon commented Oct 21, 2024

Thanks.

I'm happy to see that the integration tests fail :)
The "best" fix would be to change all the integration tests to reflect the new default, but that sounds rather painful. I think more feasible would be to indeed change the command invocation of those tests, and in addition maybe add one more case with the new defaults.

Do we change the flag description too, to something like -[no]scfix, ie. when the flag is applied scfix gets turned off?

Yes. Users need to have an option to switch the feature off.

Finally, you can combine #622 with this PR, otherwise changing the tests will result in a bunch of merge conflicts (I expect) and double work.

@csbrasnett
Copy link
Collaborator Author

Unfortunately, I think we'll have to change (at least some of) the tests. I've noticed a couple of the tests don't actually reflect the default applications. I don't think it should take too long to do 🤞

- assume scfix by default.
- make -noscfix to explictly not fix side chain angles and dihedrals
- add warning to -scfix so that if given a depreciation notice is given for now
- update tests accordingly
Copy link
Member

@pckroon pckroon left a comment

Choose a reason for hiding this comment

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

Yes, excellent!
Just some teenie tiny nitpicks

bin/martinize2 Outdated Show resolved Hide resolved
bin/martinize2 Outdated Show resolved Hide resolved
bin/martinize2 Outdated Show resolved Hide resolved
@csbrasnett
Copy link
Collaborator Author

Not sure why the lint is still failing?

@pckroon
Copy link
Member

pckroon commented Oct 24, 2024

Probably because 9.0 is not more than 9.0 or something dumb. We need to update the pylint config file as well it seems, lots of deprecated options.
Doesn't really matter at the moment though.
Excellent work!

@pckroon pckroon merged commit 53c991d into marrink-lab:master Oct 24, 2024
9 of 10 checks passed
@csbrasnett
Copy link
Collaborator Author

aha thanks! I'll have a look at #616 to check if that's ready to merge too and then we're set

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