Interface re-design: state and Lux model

[CedricTravelletti]

This PR is an overhaul of the interface to accomodate for state and other advanced uses of calculators. It supersedes #12 .

Conclusions from the last meeting are summarized below. Please feel free to edit them.

Summary of the conslusions

• On the units side
  • calculators have to specify the units they expect. They specify the enery and length units, not the force. Then we provide ftype(system, calculator), with defaults. The floating point type is that of the system passed.
  • we add zero_forces, zero_energy, zero_virial, with one line defaults that can be overloaded.
  • we provide a wrapper calculator that does the conversions for users that do not want to.
• State setters and getters
  • if you parallelize, its your task to handle the state
  • we should document that with a good example
• compute for multiple outputs:
  • we agree that this should be possible, but leave the question of adapting the macro for automatic generation to another issue.
  • could have boilerplate that computes separately and returns the tuple.
• Philosophy behind compute
  • This should be extendible and for the moment the helpers only support a limited subset, for the rest, there are no helpers.
  • Group of properties are just another type. If I want to support it (as a Calculator developper), then I can. The macros will only support the basic cases, for the rest, I am on my own for generating other calls.
  • parameters and state should not be forced upon someone => keep it in the interface, but default to nothing.
  • we behave as if we follow Lux, even if we are mutating in the background (follow the convention). The calculator must act as immutable, even if its not.
  • Document the minimal interfaces that is useful (easy prototypes).
  • Then document the advanced use separately (parametrized calculator, state, ...)
• pre-allocating forces
  • postponed to a separate PR, open an issue summarizing the discussion behind this.
  • our philosophy is that this is a "buy-in" thing, i.e. an optimization that can be used or not.
  • Michael suggests the interface from DiffResults.jl: Allow the Forces type to contain the container that should contains where we should write.
• Ordering and Deconstruction of the Results: => we do not guarantee the order. It must only contain the requested properties.

[cortner]

Thank you. Looks like an accurate summary.

[cortner]

I'm not sure the virial definition in wikipedia is that helpful, in fact it is confusing. Should we add a section to the docs that clearly specifies how the virial is defined? I.e. derivative of potential energy w.r.t. to a dimensionless deformation the cell (together with the atoms)

[cortner]

did we agree on set_state! and set_params! or set_state / set_params? Is there a way to provide both options?

[cortner]

Should Energy be PotentialEnergy? Or alternatively replace potential_energy with energy? I can see advantages and disadvantages both wauys.

[cortner]

@CedricTravelletti -- a first complete draft of the interface is ready. It would be good if you can take do an iteration to see if you spot any problems, in particular am I inconsistent anywhere with what we agreed? Am I missing key points?

Please note that I've removed a fair bit of text from the docs and instead started drafting inline docs and added links to those. I think this will make the interface definition section much easier to read.

@tjjarvinen and/or @CedricTravelletti

• please update the example to bring it in line with the modified interface
• please draft the utilities section.

I think to complete this PR only the following is missing:

• decision on set_parameters! vs set_parameters, or have both, and same for states.

Everything else, including polishing the documentation, can be postponed to future PRs. It would be good to agree on the function prototypes and merge so that progress can be made in other packages.

[CedricTravelletti]

@cortner Thanks! I will take a pass at it during this week.

I am also working on adding examples that demonstate the cases we want to adress (stateful, parametrizable, ...).

No strong opinions on set vs set!, even if I prefer the non-mutating one.

[cortner]

Ongoing TODO List

• extensive inline docs for every interface function

[cortner]

No strong opinions on set vs set!, even if I prefer the non-mutating one

Just having a chat with @tjjarvinen -- we are recommending the following

set_parameters!(calc, ps) -> calc_new
set_state!(calc, st) -> calc_new

both can but need not be mutating. The caller should not assume that calc and calc_new are the same object.

[CedricTravelletti]

@cortner @tjjarvinen Thanks! I think what you did looks great. On my side I think this is ready for merge, and adding more examples for the new functionalities becomes a separate issue.

I just fixed the tests to include the now required AtomsCalculators.energy_unit and AtomsCalculators.length_unit functions.

[cortner]

I think we do need to clarify in the documentation that the set! functions may be but need not be mutating.

[tjjarvinen]

I fixed some issues. I try to finish my review asap. There are still couple of issues I need to check.

[CedricTravelletti]

I agree with @tjjarvinen on #16

[tjjarvinen]

For my part this is in a good shape and ready for a merge.

The only question I have is: are we providing defaults for energy_unit and length_unit or leave them undefined?

[cortner]

personally I prefer to keep them undefined - force implementers to provide it.

[tjjarvinen]

I have no hard preference here. The only small thing is that, if we define defaults, then high-level interface code that use the old version would work straight. If we don't, then they would need an update.

[cortner]

I'll wait for others to comment. I just find it very odd to provide a default when different communities use different units and each are equally legitimate...

[CedricTravelletti]

I agree with @cortner on this one, plus I do not see how we could provide defaults that cover all potential realistic cases.

Apart from this, I think we are ready for merge.

[cortner]

@mfherbst @rkurchin @jgreener64 -- could at least two of you approve this PR please so we can go ahead and merge it?

[jgreener64]

I haven't looked at the code in detail, but the overall changes here look good to me.

[mfherbst]

Sorry for the delay !

No blockers from my end, just some nits.

[mfherbst]

I would suggest to be more specific here. Catch all can lead to very hard to debug error suppressions.

[cortner]

are you asking to specify which exception to catch? I don't have too strong a view here but I guess the message could be more specific, i.e. it should state what test fails and then print the exception so one can see during debugging what went wrong?

[mfherbst]

Yes I'm worried about the exception catching.

[cortner]

@tjjarvinen or @CedricTravelletti -- not sure which of you wrote this, but can you please push a fix?

[tjjarvinen]

I think you are now missing the context here. What that code actually does is

try 
        calculator_type = get_calculator_type(expr, type)
    catch _
        error("Could not determine calculators type")
end

Or basically it wraps an error with a better description on what is happening, so that it is easier to debug. Rather than throwing an error message that user does not understand where it comes from.

[cortner]

@mfherbst --- can you please comment. Personally it would be enough for me to just give a better error message, e.g. to say explicitly exactly what failed. This is something that cannot currently be seen from "Could not determine calculators type". But maybe you had more in mind.

[mfherbst]

I think that is a good start. My main worry is that someone makes a typo in the implementation of their calculator thereby triggering an exception at macro evaluation. Instead of standard error message they see this and line numbers and stack trace now point at the wrong spot. If that's not the case I'm ok.

[tjjarvinen]

I improved the error message. It now suggest that there is a typo in function definition related to the calculator type.

This should be now clear enough error

try 
    calculator_type = get_calculator_type(expr, type)
catch _
    error("Possible typo (or other issue) in function definition. Could not determine the calculators type.")
end

[cortner]

@tjjarvinen - thanks for trying to fix this. I'm not convinced though that this addresses @mfherbst concern. Maybe he can comment. Maybe what could be done is to do @error instead of error and then rethrow the exception. That way the developer can see the stacktrace and see what went wrong.

[mfherbst]

I think it's a lot better now. I've taken the liberty to merge this to not have this drag on too long. I've opened #23.

[rkurchin]

LGTM!

[mfherbst]

Lgtm

[cortner]

Thanks for merging. Can somebody update the version and register? (I'm on my phone...)

[tjjarvinen]

UnitfulAtomic does not have version bound yet. It is also only used in the example, so it is not needed either. That is a one thing that missed the review. It does block registration of new version though... Note to @mfherbst, as you are doing the registration.