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

Reduce the state machine in parameter derivative routines needed #5292

Open
ye-luo opened this issue Jan 29, 2025 · 11 comments
Open

Reduce the state machine in parameter derivative routines needed #5292

ye-luo opened this issue Jan 29, 2025 · 11 comments

Comments

@ye-luo
Copy link
Contributor

ye-luo commented Jan 29, 2025

The state machine was invented for WFOpt.
checkInVariables queries all the optimizable pieces in TWF and then we create a globalVars
checkOutVariables updates the per piece myVars by storing its index in the globalVars.
During evaluateDerivatives(globalVars), myVars and its indices are used.

Clearly the global indices stored in all the per piece myVars are the state machine. A solution is to keep the action of checkOutVariables within evaluateDerivatives and then it can be paired with arbitrary amount of globalVars which extends this machinery to estimators and even any number of estimators enabled in a single run.

Each feature that needs evaluateDerivatives calls simply register its own FeatureVars by calling checkInVariables.

@ye-luo ye-luo changed the title Reduce the state machine in parameter derivative routines Reduce the state machine in parameter derivative routines needed Jan 29, 2025
@jtkrogel
Copy link
Contributor

Thanks for thinking this through. I agree the current implementation is a mess.

How much effort do you think this would be? Would this still allow parameter derivatives to be called at the WavefunctionComponent level?

@PDoakORNL
Copy link
Contributor

My main concern is that we have been trying to keep the data dependence of estimators intelligible, thats less of a concern if these are just a context available only to the self healing estimator that can be supplied once per section or once per block. I don't have a ton of time today but aren't the variables basically owned by the BaseCostFunction? Are we talking about making a reference to these available to the SelfHealingEstimator or a different set of them? When you say evaluate I assume you mean once per measurement i.e. in the fully concurrent accumulate.

@jtkrogel
Copy link
Contributor

jtkrogel commented Jan 30, 2025

I don't think this would change the data dependence. This estimator currently (and still would) depend primarily on the trial wavefunction. The only main difference is requiring parameter derivatives to be correct at call time in an estimator just as they are with electron coordinate derivatives.

@PDoakORNL
Copy link
Contributor

PDoakORNL commented Jan 30, 2025

If you need to access them inside the accumulate then the data dependence has changed. At what scope are the derivatives the accumulation depends on mutable? Or are we talking about a list of activate variables that the estimator uses to call the evaluateDerivatives every accumulation. Apologies for not keeping up with the self healing methodology.

@ye-luo
Copy link
Contributor Author

ye-luo commented Jan 30, 2025

My intention was to resolve the implicit dependency of myVars on globalVars. Once that is resolved, evaluateDerivatives can be treated like a const function that can be called as many times we need. Thus places which need derivatives may call freely whereas currently only WFOpt can call derivative routines.

Regarding intelligent data sharing between estimators can be viewed as an optimization that eliminates redundant calculation. We can do that once we have the basic implementation working that allows evaluating derivatives in each estimator.

@PDoakORNL evaluateDerivatives(globalVars) looks like you can use abitrary globalVars. However, globalVars includes all the parameters of the whole TWF tree. Each component evaluateDerivatives relies on its class member myVars to find how the index of its parameters in the globalVars. This made it impossible to ad-hoc switch globalVars. It is all about the evaluateDerivatives API, how estimator consumes globalVars for its purpose is a separate story.

@ye-luo
Copy link
Contributor Author

ye-luo commented Jan 30, 2025

Thanks for thinking this through. I agree the current implementation is a mess.

How much effort do you think this would be?

A quick and dirty work would be moving the checkOut into each evaluateDerivatives call.
This will probably make things working. However, chcekOut is not cheap when parameter count is large since it is doing linear search. It will better if we handle parameters by groups instead of individuals. A week or two should be sufficient to restructure the offending code. An accurate answer requires the first quick and dirty experiment.

Would this still allow parameter derivatives to be called at the WavefunctionComponent level?

That is one of the goal. So yes.

@PDoakORNL
Copy link
Contributor

Ok I did not really see how tricky this was, I get the difficulty now.

@jtkrogel
Copy link
Contributor

Can we get around the linear search by using hash tables instead? Or is some type of index mapping array better?

@prckent
Copy link
Contributor

prckent commented Jan 30, 2025

Is there evidence that the linear search is unacceptably slow? (Looking for the fastest route to get something working here. Optimize later when proven to be needed).

@jtkrogel
Copy link
Contributor

Yes, agreed. We should prioritize getting to first functioning over speed.

@jtkrogel
Copy link
Contributor

jtkrogel commented Feb 6, 2025

I will note that two parameter derivatives are needed: dPsi/dp and dE_local/dp.

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

4 participants