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

Allow more transparent modification of damping parameters #98

Open
awvwgk opened this issue May 4, 2021 · 6 comments
Open

Allow more transparent modification of damping parameters #98

awvwgk opened this issue May 4, 2021 · 6 comments
Labels
API Affects the C-binding layer. wontfix This will not be worked on

Comments

@awvwgk
Copy link
Member

awvwgk commented May 4, 2021

Is your feature request related to a problem? Please describe.
API objects handling damping parameters are currently opaque, i.e. once created it is unknown which damping parameters they actually contain.

The API allows two ways to construct damping parameters right now, once via a method name or via a complete set of damping parameters (s6, s8, s9, a1, a2, alp). The Python API is lumping the two ways together and implements an ad hoc selection here, preferring the method name over custom, maybe incomplete, parameters:

def __init__(self, *, method=None, **kwargs):
"""Create new damping parameter from method name or explicit data"""
if method is not None:
_method = _ffi.new("char[]", method.encode())
self._param = load_rational_damping(
_method,
kwargs.get("s9", 1.0) > 0.0,
)
else:
try:
self._param = new_rational_damping(
kwargs.get("s6", 1.0),
kwargs["s8"],
kwargs.get("s9", 1.0),
kwargs["a1"],
kwargs["a2"],
kwargs.get("alp", 16.0),
)
except KeyError as e:
raise RuntimeError("Constructor requires argument for " + str(e))

Describe the solution you'd like
Not sure, allow to query the object?

Describe alternatives you've considered
Make the object completely transparent (bind(C) derived type).

Additional context
The API object for the damping parameters must never be partially constructed, i.e. only containing some damping parameters and not all. Also, there is little sense in requesting a method and overwriting its s8 parameter blindly, in this case the user is better aware of all damping parameters and has to take full responsibility for constructing the API object.

@awvwgk awvwgk added the API Affects the C-binding layer. label May 4, 2021
@awvwgk awvwgk changed the title Allow more transparent modifcation of damping parameters Allow more transparent modification of damping parameters May 4, 2021
@loriab
Copy link
Contributor

loriab commented May 5, 2021

I agree that compared to dftd3 that used either the dftd3 -func interface or the full parameter set input file, the present API's func trumps complete/partial parameters leads to divine-user-intent logic in a lot of places in my view of the software stack (psi4 dft construction, qcengine harness, dftd4 API).

I wouldn't mind the dftd3 API demanding either func or full parameters and erroring at both. Though that's not the only solution.

At the psi4 level, I think it's worth keeping user ability to selectively overwrite a functional's default params. Especially since for dftd4 params that are approximately fixed (alpha, s6 for non-DH, iirc) are in the complete parameter list.

Being able to query what parameters just ran from a calc object returned by dftd4 would be great. I tried to probe this once but got back opaque capsule, as you mentioned above. This would let the qcng harness (my part at least) skip the call to the great resolver fn that "reconciles" func and parameters to see if an official functional label can be applied to the results.

Thanks for bringing up this topic.

@awvwgk
Copy link
Member Author

awvwgk commented May 5, 2021

Being able to query what parameters just ran from a calc object returned by dftd4 would be great. I tried to probe this once but got back opaque capsule, as you mentioned above. This would let the qcng harness (my part at least) skip the call to the great resolver fn that "reconciles" func and parameters to see if an official functional label can be applied to the results.

I should note that the returned object is actually polymorphic and doesn't have to be a rational damping function. Kinda superfluous since we only support one damping function in dftd4 now, but judging from the number of proposed damping functions for DFT-D3 I want to be future proof here.

Accessing the damping parameters returned from dftd4 requires to first identify the kind of damping function used before being able to inspect the actual parameters. I have to think about how this can be implemented in C first.

@awvwgk
Copy link
Member Author

awvwgk commented May 5, 2021

At the psi4 level, I think it's worth keeping user ability to selectively overwrite a functional's default params. Especially since for dftd4 params that are approximately fixed (alpha, s6 for non-DH, iirc) are in the complete parameter list.

Let's clarify user here, because I'm mainly thinking API users (like psi4). How DFT-D4 is exposed in psi4 to the actual end user is not necessarily a concern of the API in dftd4. Rather I want to provide the API functionality to enable projects to provide their preferred way to use DFT-D4 to their users.

To provide the ability to selectively overwrite functional default parameters, I don't think dftd4 has to be involved directly when overwriting the parameters. Instead loading the required parameters from another source (i.e. dftd4.parameters) and than passing the final damping parameters to dftd4 seems preferable to me.

@loriab
Copy link
Contributor

loriab commented May 5, 2021

At the psi4 level, I think it's worth keeping user ability to selectively overwrite a functional's default params. Especially since for dftd4 params that are approximately fixed (alpha, s6 for non-DH, iirc) are in the complete parameter list.

Let's clarify user here, because I'm mainly thinking API users (like psi4). How DFT-D4 is exposed in psi4 to the actual end user is not necessarily a concern of the API in dftd4. Rather I want to provide the API functionality to enable projects to provide their preferred way to use DFT-D4 to their users.

To provide the ability to selectively overwrite functional default parameters, I don't think dftd4 has to be involved directly when overwriting the parameters. Instead loading the required parameters from another source (i.e. dftd4.parameters) and than passing the final damping parameters to dftd4 seems preferable to me.

I completely agree. The less logic the dftd4 API applies behind the scenes, the simpler it is to implement more involved logic like overwriting parameters in psi4. Looking up the values in dftd4.parameters and negotiating between that and any given functional name works great.

The API consistency problem I got myself into is long ago creating a psi4.Molecule.run_dftd3 fn that takes in func and/or params, does the full negotiation like psi4 and runs dftd3. Since all the logic was external to dftd3, I made the dftd3 qcng harness consistent and also run the negotiated solution on qcel.Molecule or a plain harness run. Then dftd4 comes along and does have internal logic for func vs parameters that's inconsistent with dftd3 and mp2d harnesses. Yet qcengine's mandate is to provide as transparent a pass-through as possible between AtomicInput and dftd4's usual input. Hence the complication to the harness to accommodate multiple entry points. Also your func-trumps-params I view as more in keeping with AtomicInput specification where model.method should remain ~accurate, no matter the keywords values.

I guess I can see leaving the dftd4 API as-is at func-trumps-params in keeping with AtomicInput. Or the only-func or only-params routes in keeping with dftd3. But whichever you choose, I think it's the dftd4 harness's role to follow it and not overwrite the interface.

@awvwgk
Copy link
Member Author

awvwgk commented May 24, 2021

Just a quick followup on the possibility of implementing this in the C API.

I started a thread in the Fortran discourse on the best way to handle polymorphic objects in a C API situation: https://fortran-lang.discourse.group/t/exposing-polymorphic-objects-to-c/1290

While there seems to be a way, the overall recommendation is to reconsider the API. I'm not going to risk API breakage for this feature, but there might be another way. One option would be to add a new C API call to retrieve damping parameters from the internal storage and use this function to implement the dftd4_load_rational_damping C API call under the hood. The Python API could load the parameters this way, replace individual values with overwrites and than call the normal constructor with dftd4_new_rational_damping. However, this is just a complicated version of what is implemented in the dftd4.parameters module already.

I'll mark it as wontfix for now, since there is some way to archive the desired behavior without touching the C API available.

@awvwgk awvwgk added the wontfix This will not be worked on label May 24, 2021
@awvwgk
Copy link
Member Author

awvwgk commented Nov 20, 2021

I'm planning to slightly change the behaviour of the damping function constructor in #137 and make it an error to provide both method name and explicit damping parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Affects the C-binding layer. wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants