-
Notifications
You must be signed in to change notification settings - Fork 440
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
Deprecate Built-In ADC #2419
Deprecate Built-In ADC #2419
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Just as a remark: I suggest to default to adcc in procrouting as well for ADC(2) in case it is available. Right now the default is still the builtin.
I'm in favor, but this sounds like the sort of thing other developers may object to, so I'll defer to popular opinion before deciding to adopt or not. |
Could we catch the case where users request multiple roots and stop the program? Fewer and fewer people look at the actual output files. I would also support the more extreme solution of just fully disabling the internal adc. Better some potential inconvenience than wrong results. |
|
Well since the argument came up again: I'm all for removal, because with The step to go from requesting a psi-internal ADC calc to having the same job running with IMHO there should not be a "deprecation phase" for a feature which is obviously broken and a full replacement is already there. |
I suppose if people want to get the "broken" version of ADC, they can install Psi4 between (at least) 1.3.2 and 1.5, right? I don't think we need to deprecate features that don't work, and if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see 3 levels of adc deprecation:
- warn on build-in adc invocation and leave adc as default
- warn on built-in adc invocation and switch adcc to default (managed_adcc, as @mfherbst suggests)
- rm -rf adc/
Right now this PR does (1). I don't remember if @fevangelista was against (2) or only against (3). I'm against (3) until at least after 1.6 release. On the whole, I favor (2) now, but this PR of (1) now is an excellent first step.
Ok, (2) is a solution I can live with 😄 Then at least the ADC calculations from 1.6+ won't be wrong by default 😉:+1: |
(2) was never discussed on the conference call, so I'll take that option for this PR. The driver-side changes are straightforward, test changes should be as well. |
Default switched to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have done "if module in {ADCC, ''}: run_adcc; elif module in {BUiltin}: run_adc" to avoid the complication of mixed defaults. But as-proposed satisfies the continuity camp by keeping running. And it satisfies the correctness camp by using adcc and peppering the user with adc warnings. So, lgtm!
Great, will merge on test pass. |
* Slate ADC for the end * Advance a deprecation stage
Description
This PR deprecates Psi's built-in ADC module, per discussion on the January developer conference call, issue #1033, and my own investigation.
Checklist
Status