Efficient burn in for sampling #298

vgnecula · 2024-03-26T15:55:16Z

Modified the sampling.hpp to solve: Inefficient handling of burn in phase in sampling functions #180.

Handling burn in phase without storing the points required for this phase, for each of the sampling methods presented.

Cleared my branch non-related developments in order to emphasize just the sampling.hpp code.

TolisChal · 2024-03-26T17:51:06Z

include/sampling/sampling.hpp


+ for (unsigned int i = 0; i < rnum; ++i) {
+ walk.apply(P, p, walk_len, rng); 
+ randPoints.push_back(p); 


Could you please reserve memory equal to rnum points before the push_backs? (i.e., randPoints.reserve(rnum);)

I am not able to do that directly using randPoints.reserve(rnum); as randPoints is an instance of an std::list and not std::vector. Thus it does not support this type of command. Am I missing something?

I thought about finding a workaround for reserving space from the first time, however as I figured out randPoints was a list, I decided that it is not the case anymore, as I can't get continuous storage anyway.

randPoints is templated (i.e. it can be either a std::vector or a std::list or any container that supports push_back), thus you can create a specialized version of this function that reserves memory only for the case of a vector but I propose not to do anything in this PR. We should think if we need to want to keep it as it is or restrict randPoints to std::list or std::vector.
@TolisChal do we need random access in samples? If not should we restrict to std::list here?

TolisChal · 2024-03-26T17:51:16Z

include/sampling/sampling.hpp

+
+ for (unsigned int i = 0; i < rnum; ++i) {
+ walk.apply(P, p, walk_len, rng); 
+ randPoints.push_back(p); 


same as before

TolisChal · 2024-03-26T17:51:58Z

include/sampling/sampling.hpp


+ for (unsigned int i = 0; i < rnum; ++i) {
+ walk.apply(P, p, a, walk_len, rng); 
+ randPoints.push_back(p); 


same as before

TolisChal · 2024-03-26T17:52:10Z

include/sampling/sampling.hpp

+
+ for (unsigned int i = 0; i < rnum; ++i) {
+ walk.apply(P, p, a, walk_len, rng); 
+ randPoints.push_back(p); 


same as before

include/sampling/sampling.hpp

TolisChal · 2024-03-26T18:42:02Z

@vgnecula thanks for this PR!

A general comment is, when you want to change a particular part of the code, just change this one without affecting anything else. Then, it's easier to review the code and be sure about the changes.

vfisikop

Thanks. I suggest as @TolisChal also suggested to focus on the issue of the burn in phase and leave style changes for future PR.

vfisikop · 2024-03-26T20:38:44Z

include/sampling/sampling.hpp

+ }
+ }
+
+ randPoints.clear();


clear is not needed here.

vfisikop · 2024-03-26T20:40:22Z

include/sampling/sampling.hpp

- typename RandomNumberGenerator,
- typename Point
+
+template <


I would prefer to change style at a separate PR.

vfisikop · 2024-03-26T20:42:36Z

include/sampling/sampling.hpp


+
+ randPoints.clear();


is this needed?

similar in all walks

Initially, I did not have this function. However, I thought that it might be a good practice to ensure that everything is cleared before performing the actual sampling. Besides, this operation should be done in constant time (correct me if I am wrong), especially in the case in which we expect randPoints to be empty. Consequently, I thought that there would be no problem in adding it just for safety.

vfisikop · 2024-03-26T20:43:01Z

include/sampling/sampling.hpp

+
+ randPoints.clear();
+
+ for (unsigned int i = 0; i < rnum; ++i) {


there is a mistake here, it was rnum/2 before

vfisikop · 2024-03-26T20:44:03Z

include/sampling/sampling.hpp

+
+ for (unsigned int i = 0; i < rnum; ++i) {
+ walk.apply(P, p1, p2, walk_len, rng); 
+ randPoints.push_back(p1); 


should we also push p2?

vfisikop · 2024-03-26T20:46:04Z

include/sampling/sampling.hpp


- Point p = starting_point;
+ Point p = starting_point; 


please do not add trailing whitespaces (there are more cases like this in this PR)

include/sampling/sampling.hpp

vfisikop · 2024-03-26T20:49:30Z

include/sampling/sampling.hpp

+ const NT &a,
+ Point &starting_point, 
+ unsigned int const& nburns
+) {


looks like a new style of code, I suggest using the style of most of the functions in volesti

vfisikop · 2024-03-26T20:52:09Z

Also it seems that you are deleting (by mistake) some crucial parts of the code in sampling.hpp.

vfisikop · 2024-03-27T12:46:53Z

Side comment: there is no need to open a new PR every time you want to change something in your branch, just update your branch!

vgnecula · 2024-04-08T08:47:51Z

@TolisChal @vfisikop just commited a new version, which adresses all your comments (find it as - minor probem with CRHMC solved - the last version of my new modifications). On my end it passes all tests. However, it takes way too much time to process crhmc_test_polytope_sampling. Besides, as you will probably see, I modified some functions, as I did not like some details that I implemented before.

vfisikop

I reviewed part of it, but it is difficult to continue. Could you please separate your commits such that there is one commit with the changes to burn in feasibility and another one with the style changes?

vfisikop · 2024-04-09T10:26:38Z

include/sampling/sampling.hpp

+ RandomNumberGenerator &rng,
+ const unsigned int &walk_len,
+ const unsigned int &rnum,
+ Point &starting_point, 


please avoid using trailing whitespaces.

there are similar cases below

Thank you for taking the time to review my new Commits!

I just wanted to be sure I understand what you mean by style changes.

So, I should have a first commit that for the lines where I did not change anything regarding the algorithm logic - the lines remain the same, while the second commit adds my proposed style changes.

I saw that there are some small differences in the ways some things are written and I tried to bring them all to the same writing style (that is why I made those modifications). Sorry for the white spaces, I missed some of them.

Yes, exactly, we want one commit to fix the burn in phase that does not change any coding style (i.e. indentation etc) and another commit that contains all coding styles. Also please rebase (see git rebase documentation) your branch to only have those two commits and not the current ones (currently 4).

@vfisikop should I push the "style-changes" commit? Or should I wait until the current one gets reviewed? Thank you!

The PR should consist of two commits, one with the fixes and one with the style changes. So those could be reviewed independently.

Having a look at this code again I see there are some problems with this interface, e.g. there is an issue of what walk should the user use for a particular distribution. You can have a look at this draft PR #300 for an idea of changing this to a new interface.

I just started to look into it more. Could you please elaborate a bit more on what you think I should do? I want to be sure I fully understood what you meant.

vfisikop · 2024-04-09T10:51:48Z

include/sampling/sampling.hpp

- randPoints, push_back_policy, rng);
- randPoints.clear();
+ for (unsigned int i = 0; i < nburns; ++i) {
+ walk.apply(P, current_point, dummy_point, walk_len, rng);


I think there is not reason to keep only one point, they are both on the boundary, so I propose to keep both. @TolisChal what do you think?

Yes, we could do that, however keeping only one would be closer to a uniform sample (just intuitively).
A general comment from my side is that we need to delete this method in the future and implement Shake and Bake.

vfisikop · 2024-04-09T11:00:05Z

include/sampling/sampling.hpp

-
- // Initialize random walk parameters
- unsigned int dim = starting_point.dimension();
- walk_params params(F, dim);


how you pass the parameters to the walk?

@vfisikop Thank you for pointing this out! So previously I was only relying on the assumption that the walk object will auto set the params. However after looking into how the langevin_walk is implemented, I saw that this is not the case. Thus, now I am setting the params using the set_params method. Let me know if this makes sense.

TolisChal reviewed Mar 26, 2024

View reviewed changes

include/sampling/sampling.hpp Show resolved Hide resolved

TolisChal reviewed Mar 26, 2024

View reviewed changes

include/sampling/sampling.hpp Show resolved Hide resolved

vfisikop reviewed Mar 26, 2024

View reviewed changes

vgnecula force-pushed the burnIn_handling branch from b596cd2 to 8dc74d7 Compare April 8, 2024 07:27

vfisikop reviewed Apr 9, 2024

View reviewed changes

vgnecula force-pushed the burnIn_handling branch 6 times, most recently from e19ce96 to 46d4224 Compare April 20, 2024 05:26

BurnIn Handling, same style

8367fa3

vgnecula force-pushed the burnIn_handling branch from 46d4224 to 8367fa3 Compare April 20, 2024 05:30

vgnecula closed this Jun 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Efficient burn in for sampling #298

Efficient burn in for sampling #298

vgnecula commented Mar 26, 2024

TolisChal Mar 26, 2024

vgnecula Mar 27, 2024

vgnecula Mar 27, 2024

vfisikop Mar 27, 2024 •

edited

TolisChal Mar 26, 2024

TolisChal Mar 26, 2024

TolisChal Mar 26, 2024

TolisChal commented Mar 26, 2024 •

edited

vfisikop left a comment

vfisikop Mar 26, 2024

vfisikop Mar 26, 2024

vfisikop Mar 26, 2024

vfisikop Mar 26, 2024

vgnecula Mar 27, 2024

vfisikop Mar 26, 2024

vfisikop Mar 26, 2024

vfisikop Mar 26, 2024

vfisikop Mar 26, 2024

vfisikop commented Mar 26, 2024 •

edited

vfisikop commented Mar 27, 2024

vgnecula commented Apr 8, 2024 •

edited

vfisikop left a comment

vfisikop Apr 9, 2024

vfisikop Apr 9, 2024

vgnecula Apr 9, 2024

vfisikop Apr 9, 2024

vgnecula Apr 24, 2024

vfisikop Apr 26, 2024

vfisikop Apr 26, 2024

vgnecula May 16, 2024

vfisikop Apr 9, 2024

TolisChal May 22, 2024

vfisikop Apr 9, 2024

vgnecula Apr 20, 2024


		randPoints.clear();

		for (unsigned int i = 0; i < rnum; ++i) {

Efficient burn in for sampling #298

Efficient burn in for sampling #298

Conversation

vgnecula commented Mar 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vfisikop Mar 27, 2024 • edited

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

TolisChal commented Mar 26, 2024 • edited

vfisikop 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vfisikop commented Mar 26, 2024 • edited

vfisikop commented Mar 27, 2024

vgnecula commented Apr 8, 2024 • edited

vfisikop 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

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

vfisikop Mar 27, 2024 •

edited

TolisChal commented Mar 26, 2024 •

edited

vfisikop commented Mar 26, 2024 •

edited

vgnecula commented Apr 8, 2024 •

edited