Parameter Filtration in LM Optimizer #4126

LeonOtis · 2022-07-21T04:13:33Z

Proposed changes

This is the remainder of the changes originally proposed in #4061 . This PR allows the wave function parameters optimized by the linear method to be filtered based on the amount of noise in the parameter gradients. The LM will then optimize only a subset of the parameters while the others can be handled by accelerated descent in a hybrid optimization.

The main changes are in the LM engine and the QMCFixedSampleLinearOptimizeBatched driver. There are a few additional user inputs for turning on parameter filtration, which are currently being handled similarly to the many other driver inputs. The parameter filtration should now be compatible with both MPI and OMP parallelism. Thanks again for all the feedback so far and I am always happy to make further improvements.

What type(s) of changes does this code introduce?

New feature

Does this introduce a breaking change?

No

What systems has this change been tested on?

Lawrencium computing cluster

Checklist

Yes. This PR is up to date with current the current state of 'develop'
Yes. Code added or changed in the PR has been clang-formatted
No. This PR adds tests to cover any new code, or to catch a bug that is being fixed
Yes. Documentation has been added (if appropriate)

markdewing · 2022-07-21T06:14:48Z

Test this please

PDoakORNL

Unless I'm reading this very wrong there are serious fundamental problems with the concurrency logic of the batched linear optimization driver and this PR. Please mark this as [WIP] until the concurrency issues in it are fixed.

One of the maintainers should help you work through this in detail.

Have you run this with opt_num_crowds > 1? I would expect something interesting to happen.

src/QMCDrivers/WFOpt/EngineHandle.h

src/QMCDrivers/WFOpt/QMCCostFunctionBatched.cpp

src/QMCDrivers/WFOpt/QMCFixedSampleLinearOptimizeBatched.cpp

src/formic/utils/lmyengine/engine.cpp

src/QMCDrivers/WFOpt/EngineHandle.h

src/QMCDrivers/tests/test_EngineHandle.cpp

src/QMCDrivers/WFOpt/EngineHandle.h

src/QMCDrivers/WFOpt/QMCCostFunctionBatched.cpp

LeonOtis · 2022-08-01T05:54:27Z

Thanks for the feedback. The new set of changes have the history arrays in the LM engine sized according to the number of samples on an MPI rank and different threads should store data in them using the index offsets. Tests I've done with multiple threads have worked, but there could be cases I've overlooked. I'm happy to do further rounds of fixes as needed.

src/QMCDrivers/WFOpt/EngineHandle.h

markdewing · 2022-08-02T15:45:06Z

Do you have an example input file for this feature? Ideally there should be a test added that also serves as an example. However, parameter filtering gets used when scaling up the number of parameters than can be handled and a test that is small enough to be a good test might not be a good example. It also sounds like parameter filtering gets used in combination with hybrid optimization. It would be good to have a more complete example to see how the feature works in use. Attaching files to the PR is good enough for now.

LeonOtis · 2022-08-02T16:55:34Z

example_input.zip

Sure, here are input files for a quick example with the carbon trimer. The actual optimization results won't be very meaningful with low sample numbers and filtering an already small number of parameters, but the example_filter_hybrid.xml file shows the new inputs in the context of a hybrid optimization. Parameter filtration can be used for the LM on its own just by using the LM section of the hybrid input as a standalone, but I would generally expect the results to be inferior to allowing descent to optimize any parameters the LM leaves alone. I think this PR should already have some changes to the documentation to explain the new inputs, but I can add more if needed.

LeonOtis · 2022-08-08T23:34:40Z

Added a small fix to the legacy driver to correct a problem @ye-luo noticed with PR #4075. At this point, what else is needed before this PR can be merged? I'm happy to revise the handling of threading more if desired. I'm also willing to come up with tests if needed though I wanted to be sure the threading issues were settled before trying to design any.

ye-luo · 2022-08-08T23:47:56Z

@LeonOtis could you make a new branch from develop and cherry pick the last fix commit you pushed and then make a separate PR.

LeonOtis · 2022-08-09T00:25:45Z

@ye-luo Ok, I've opened PR #4161 for that small change.

LeonOtis · 2022-08-12T18:16:40Z

@ye-luo The hybrid method should work with the batched driver. I've now added a test for it on H4, similar to what I previously added for the legacy driver.

ye-luo · 2022-08-12T18:26:34Z

@ye-luo The hybrid method should work with the batched driver. I've now added a test for it on H4, similar to what I previously added for the legacy driver.

Thank you. Could you make a separate PR with the new commits to develop? Since it works with the develop already.

LeonOtis · 2022-08-12T18:53:16Z

@ye-luo The hybrid method should work with the batched driver. I've now added a test for it on H4, similar to what I previously added for the legacy driver.

Thank you. Could you make a separate PR with the new commits to develop? Since it works with the develop already.

Ok, I've opened a new PR #4172 that just adds the test.

ye-luo

@LeonOtis I did some cleaning, please pull before continuing editing. Once my requests are addressed, I think this PR can get in. I close my eyes on run_three_shift and everything inside formic.

src/QMCDrivers/WFOpt/QMCFixedSampleLinearOptimizeBatched.cpp

src/QMCDrivers/WFOpt/QMCFixedSampleLinearOptimizeBatched.h

src/QMCDrivers/WFOpt/QMCCostFunctionBatched.cpp

ye-luo

There is still an issue of explicitly using threads. That is not a good pattern. However, I'd like that being addressed in a later PR.

src/QMCDrivers/WFOpt/EngineHandle.h

ye-luo · 2022-08-19T01:12:18Z

src/QMCDrivers/WFOpt/EngineHandle.h

+        le_der_samp[j + 1]  = static_cast<FullPrecValue>(dhpsioverpsi_array.getValue(j, local_index)) +
+            le_der_samp[0] * static_cast<FullPrecValue>(dlogpsi_array.getValue(j, local_index));
+      }
+      int ip = omp_get_thread_num();


would like to move away from thread_num

ye-luo · 2022-08-19T01:14:03Z

Ping @PDoakORNL @markdewing since you gave review comments as well.

…rt_two_param_filter

PDoakORNL

I think some of the thread safety issues I previously was worried have been cleaned up but the subtypes of EngineHandle seem to be trouble and perhaps only one is really relevant to this PR.

DescentEngineHandle/Descent engine seems incoherent, if this is just here to get things compiling keep the unfinished code in your private branch and remove it from the PR throw a runtime exception if anyone tries to use it.

That would get us to smaller issues like the anonymous struct.

PDoakORNL · 2022-08-22T21:41:50Z

src/QMCDrivers/WFOpt/QMCCostFunctionBatched.cpp

@@ -280,6 +278,7 @@ void QMCCostFunctionBatched::checkConfigurations(EngineHandle& handle)
  std::vector<int> samples_per_crowd_offsets(opt_num_crowds + 1);
  FairDivide(rank_local_num_samples_, opt_num_crowds, samples_per_crowd_offsets);

+  handle.prepareSampling(NumOptimizables, rank_local_num_samples_);


From what I can see this method should not take the EngineHandle reference handle as an argument because most of the classes using that BaseClass are incompatible with the batched concurrency model and don't infact have the same sematics for the prepareSampling and takeSample Methods.

I'm not getting this part "don't infact have the same sematics for the prepareSampling and takeSample Methods."

It doesn't make any sense if engine handle is of subtype NullEngineHandle and DescentEngineHandle in which case omp_max_num_threads is still used to prepareStorage and the num_samples parameter to the prepareStorage is simply ignored.
And In the case of the DescentEngineHandle it ignores the base_sample_index input.
I would call this having different semantics.

src/QMCDrivers/WFOpt/QMCFixedSampleLinearOptimizeBatched.cpp

PDoakORNL · 2022-08-22T21:47:17Z

src/QMCDrivers/WFOpt/EngineHandle.h

@@ -92,20 +93,24 @@ class DescentEngineHandle : public EngineHandle
  void takeSample(const std::vector<FullPrecReal>& energy_list,
                  const RecordArray<Value>& dlogpsi_array,
                  const RecordArray<Value>& dhpsioverpsi_array,
-                  int ib) override
+                  int base_sample_index) override


So this isn't used?

PDoakORNL · 2022-08-22T22:05:03Z

src/QMCDrivers/WFOpt/QMCFixedSampleLinearOptimizeBatched.h

-  OptimizerType current_optimizer_type_;
+  //String inputs are listed outside the struct
+  ///name of the current optimization method, updated by processOptXML before run
+  std::string MinMethod;


the naming of all these private variables does not follow the coding standards of QMCPACK.

src/QMCDrivers/WFOpt/QMCFixedSampleLinearOptimizeBatched.h

PDoakORNL · 2022-08-22T22:23:56Z

src/QMCDrivers/WFOpt/EngineHandle.h

+            le_der_samp[0] * static_cast<FullPrecValue>(dlogpsi_array.getValue(j, local_index));
+      }
+      int ip = omp_get_thread_num();
+      engine_.takeSample(ip, der_rat_samp, le_der_samp, le_der_samp, 1.0, 1.0);


How this relates to all the replica based accesses in the DescentEngine itself is really cryptic. Since this oenpmp thread id which isn't guaranteed to be any particular number especially in the batched context yet it is used as the replica id in the engine, which seems like it is very important.

src/QMCDrivers/WFOpt/EngineHandle.h

PDoakORNL

Ok another iteration to go here. Some of these broken Engines/EngineHandles and elements of formic could just be documented including todo's But by the end of this series of PR's the illogical and/or wrong code needs to be gone. Especially since sizable amounts of it appear to be new.

PDoakORNL · 2022-08-25T16:20:51Z

src/QMCDrivers/WFOpt/QMCCostFunctionBatched.cpp

@@ -280,6 +278,7 @@ void QMCCostFunctionBatched::checkConfigurations(EngineHandle& handle)
  std::vector<int> samples_per_crowd_offsets(opt_num_crowds + 1);
  FairDivide(rank_local_num_samples_, opt_num_crowds, samples_per_crowd_offsets);

+  handle.prepareSampling(NumOptimizables, rank_local_num_samples_);


It doesn't make any sense if engine handle is of subtype NullEngineHandle and DescentEngineHandle in which case omp_max_num_threads is still used to prepareStorage and the num_samples parameter to the prepareStorage is simply ignored.
And In the case of the DescentEngineHandle it ignores the base_sample_index input.
I would call this having different semantics.

src/QMCDrivers/WFOpt/QMCFixedSampleLinearOptimizeBatched.h

src/QMCDrivers/tests/test_EngineHandle.cpp

src/formic/utils/lmyengine/engine.cpp

PDoakORNL · 2022-08-25T17:03:26Z

src/formic/utils/lmyengine/matrix_builder.h

+    void resetParamNumber(int new_num)
+     {
+     _num_params = new_num;
+     int NumThreads = omp_get_max_threads();


The is an abstraction to get the number of threads, see src/Concurrency/Info.hpp but the optimization has no guarantee it will run on that many threads so this just isn't an effective way to allocate concurrenct workspace. It's possible this will work for awhile but it is at least a memory use bomb.

#4209 tracks the issue

ye-luo · 2022-08-25T18:54:36Z

Test this please

LeonOtis added 11 commits July 11, 2022 14:04

Add inputs for param filtration

0af0df1

Add filtration functions to LM engine

066ae52

Modify LM driver to account for varying param num

7db9d4f

Small fix to hybrid method

6764eea

Add documentation

ca320fd

Fix interface for param filter case

12cd8c4

Remove accidentally added swp files

e6b5b40

Make param filtration compatible with threading

127266e

Clean up unused functions and add comments

05a4eed

Merge in changes from develop.

6904664

Add missing comma in bibtex

edcc22d

PDoakORNL requested changes Jul 21, 2022

View reviewed changes

markdewing reviewed Jul 22, 2022

View reviewed changes

src/QMCDrivers/WFOpt/QMCCostFunctionBatched.cpp Outdated Show resolved Hide resolved

LeonOtis added 4 commits July 29, 2022 14:02

Fix thread safety

1bde8e6

Minor cleanup

01df3c7

Change history array sizes and have threads store using index offsets

9c7dcf2

Merge in changes on develop

acfe7c8

markdewing reviewed Aug 1, 2022

View reviewed changes

src/QMCDrivers/WFOpt/EngineHandle.h Outdated Show resolved Hide resolved

Change naming for sample index

a054aa1

Fix hybrid method for legacy driver

79987fa

LeonOtis mentioned this pull request Aug 8, 2022

Interfacing Optimizers with Batched Drivers #4075

Merged

LeonOtis added 2 commits August 10, 2022 22:03

Add unit test for parameter filtration

b6590dd

Merge in changes on develop.

26dee7a

LeonOtis added 2 commits August 11, 2022 15:09

Add ifdef to unit test for build cases without LMYEngine

fb1d696

Add test for hybrid method with batched driver

962cceb

ye-luo added 4 commits August 16, 2022 09:51

Formatting

77b49db

Merge remote-tracking branch 'origin/develop' into pr/origin-4126

202112a

Remove unused arguments.

83fc24e

More formatting.

42df18a

ye-luo reviewed Aug 16, 2022

View reviewed changes

LeonOtis and others added 4 commits August 18, 2022 16:04

Group optimizer input params in a struct

e964b8c

Handle ib loop inside takeSample

710633a

Merge branch 'develop' into part_two_param_filter

135a8ab

Formatting.

8709e7f

ye-luo mentioned this pull request Aug 19, 2022

ParameterSet cannot handle boolean #2758

Closed

ye-luo previously approved these changes Aug 19, 2022

View reviewed changes

LeonOtis added 2 commits August 19, 2022 14:29

Merge branch 'develop' of https://github.com/LeonOtis/qmcpack into pa…

88a4f42

…rt_two_param_filter

Remove string input for setting boolean values

2c8e710

LeonOtis dismissed ye-luo’s stale review via 2c8e710 August 19, 2022 23:00

PDoakORNL requested changes Aug 22, 2022

View reviewed changes

Name struct as LMYOptions

59dce50

PDoakORNL requested changes Aug 25, 2022

View reviewed changes

ye-luo added 4 commits August 25, 2022 13:14

Address review

a462563

Merge remote-tracking branch 'origin/develop' into pr/origin-4126

8f36411

Rename variables in tests.

6184a1c

Rename to options_LMY_

9dc32eb

PDoakORNL approved these changes Aug 25, 2022

View reviewed changes

ye-luo merged commit 4b57d30 into QMCPACK:develop Aug 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameter Filtration in LM Optimizer #4126

Parameter Filtration in LM Optimizer #4126

LeonOtis commented Jul 21, 2022

markdewing commented Jul 21, 2022

PDoakORNL left a comment

LeonOtis commented Aug 1, 2022

markdewing commented Aug 2, 2022 •

edited

Loading

LeonOtis commented Aug 2, 2022

LeonOtis commented Aug 8, 2022

ye-luo commented Aug 8, 2022

LeonOtis commented Aug 9, 2022

LeonOtis commented Aug 12, 2022

ye-luo commented Aug 12, 2022

LeonOtis commented Aug 12, 2022

ye-luo left a comment •

edited

Loading

ye-luo left a comment

ye-luo Aug 19, 2022

ye-luo commented Aug 19, 2022

PDoakORNL left a comment

PDoakORNL Aug 22, 2022

ye-luo Aug 22, 2022

PDoakORNL Aug 25, 2022

PDoakORNL Aug 22, 2022

PDoakORNL Aug 22, 2022

PDoakORNL Aug 22, 2022

PDoakORNL left a comment

PDoakORNL Aug 25, 2022

PDoakORNL Aug 25, 2022

ye-luo Aug 25, 2022

ye-luo commented Aug 25, 2022

Parameter Filtration in LM Optimizer #4126

Parameter Filtration in LM Optimizer #4126

Conversation

LeonOtis commented Jul 21, 2022

Proposed changes

What type(s) of changes does this code introduce?

Does this introduce a breaking change?

What systems has this change been tested on?

Checklist

markdewing commented Jul 21, 2022

PDoakORNL left a comment

Choose a reason for hiding this comment

LeonOtis commented Aug 1, 2022

markdewing commented Aug 2, 2022 • edited Loading

LeonOtis commented Aug 2, 2022

LeonOtis commented Aug 8, 2022

ye-luo commented Aug 8, 2022

LeonOtis commented Aug 9, 2022

LeonOtis commented Aug 12, 2022

ye-luo commented Aug 12, 2022

LeonOtis commented Aug 12, 2022

ye-luo left a comment • edited Loading

Choose a reason for hiding this comment

ye-luo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ye-luo commented Aug 19, 2022

PDoakORNL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PDoakORNL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ye-luo commented Aug 25, 2022

markdewing commented Aug 2, 2022 •

edited

Loading

ye-luo left a comment •

edited

Loading