Fb fix pso memory #487

tarcisiofischer · 2021-10-05T18:40:01Z

Related to #486

…re equal

src/algorithms/pso.cpp

include/pagmo/algorithms/pso.hpp

bluescarni · 2021-10-06T08:08:27Z

include/pagmo/algorithms/pso.hpp

@@ -254,9 +267,10 @@ class PAGMO_DLL_PUBLIC pso
    unsigned m_neighb_param;
    // memory
    bool m_memory;
+    mutable std::optional<pso::memory> m_memory_data;


std::optional might be problematic because I don't think it is supported by the Boost.serialization library. If this is indeed the case, replacing it with boost::optional should be straightforward.

I've changed this to boost::optional, so that I can use #include <boost/serialization/optional.hpp>. Worked as expected :)

src/algorithms/pso.cpp

bluescarni · 2021-10-06T08:15:21Z

@tarcisiofischer thanks for the PR!

It's a +1 from me on the coding side, I'll let @darioizzo review the algorithmic part.

This is still missing serialization support though. You should make the new memory struct serializable and then ensure that the new pso data member std::/boost::optional<memory> is serialised together with the other data members. Please let me know if you need help with this part.

I was also thinking that perhaps as an alternative to the optional approach, you could use std::unique_ptr instead. This way the definition of the memory struct could go in the pso.cpp file using the pimpl pattern, in order to minimise the changes to the header file.

tarcisiofischer · 2021-10-06T13:10:45Z

@tarcisiofischer thanks for the PR!

It's a +1 from me on the coding side, I'll let @darioizzo review the algorithmic part.

Nice thanks @bluescarni :)

This is still missing serialization support though. You should make the new memory struct serializable and then ensure that the new pso data member std::/boost::optional<memory> is serialised together with the other data members. Please let me know if you need help with this part.

I've implemented the serialization support. It was really straightforward, but perhaps I did missed something. Please take a look!

I was also thinking that perhaps as an alternative to the optional approach, you could use std::unique_ptr instead. This way the definition of the memory struct could go in the pso.cpp file using the pimpl pattern, in order to minimise the changes to the header file.

Hmm... Are you concerned about the memory struct in the hpp file due to possible increase of compile time? Can you confirm I should go for it and remove the optional? (I ask because I prefer the optional solution, but at the same time I understand that this is not my code hehe Just double checking that this is important enough and I should really do it!)

include/pagmo/algorithms/pso.hpp

bluescarni · 2021-10-06T19:17:20Z

I've implemented the serialization support. It was really straightforward, but perhaps I did missed something. Please take a look!

It looks good. Normally adding serialisation via Boost.serialisation is rather mechanical (one just needs to make sure that all data members are indeed included in the serialisation call).

Hmm... Are you concerned about the memory struct in the hpp file due to possible increase of compile time? Can you confirm I should go for it and remove the optional? (I ask because I prefer the optional solution, but at the same time I understand that this is not my code hehe Just double checking that this is important enough and I should really do it!)

No it's fine, it's just that at one point I went through a major refactor moving the library from header-only to compiled and since those days I have been a bit obsessed about reducing the code in the header files :)

bluescarni · 2021-10-06T19:20:47Z

The docs build failure is due to a web link that became dead. I'll fix it later.

bluescarni · 2021-10-06T20:08:31Z

The other two build failures look genuine. Could you fix them?

tarcisiofischer · 2021-10-07T11:35:48Z

src/algorithms/pso.cpp

@@ -46,6 +46,10 @@ see https://www.gnu.org/licenses/. */
 #include <pagmo/types.hpp>
 #include <pagmo/utils/generic.hpp>

+// NOTE: apparently this must be included *after*


I find out that this is a "common problem" inside pagmo. This include block (comment+include) is copy-pasted in several algorithms. In order to maintain the pattern, I decided to copy-paste it here too.

The Boost.serialisation library is unfortunately sensitive to the header order inclusion when it comes to the serialisation of polymorphic classes via base pointers (which we need in the implementation of pagmo's type-erased wrappers).

tarcisiofischer · 2021-10-07T12:12:01Z

The docs build failure is due to a web link that became dead. I'll fix it later.

I can do it, but I need some help here.
I believe you refer to this:

(docs/cpp/utils/genetic_operators: line   61) broken    https://www.iitk.ac.in/kangal/papers/k2012016.pdf - 404 Client Error: Not Found for url: https://www.iitk.ac.in/kangal/papers/k2012016.pdf

Which seems to be automatically generated from utils/genetic_operators.cpp.
The difficult part here is to know what to do. Link seems to be indeed broken, and I don't know if I can get it from somewhere else...

Other than that, I believe I'm done here. Let me know if anything else is necessary, and thanks again for all the support and feedbacks!

bluescarni · 2021-10-07T12:14:36Z

@tarcisiofischer googling the PDF filename leads to this:

https://dl.acm.org/doi/10.1504/IJAISC.2014.059280

Let me see if I can edit it directly from the github interface...

bluescarni · 2021-10-07T12:16:08Z

Nope... @tarcisiofischer do you mind replacing the link on the fly?

bluescarni · 2021-10-07T12:22:11Z

@tarcisiofischer cheers and thanks for the PR! I am ok with merging as soon as @darioizzo takes a second look.

darioizzo · 2021-10-07T16:48:33Z

yes, please let me have a look before merging ... will do so this weekend

…

On Thu, 7 Oct 2021, 14:22 Francesco Biscani, ***@***.***> wrote: @tarcisiofischer <https://github.com/tarcisiofischer> cheers and thanks for the PR! I am ok with merging as soon as @darioizzo <https://github.com/darioizzo> takes a second look. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#487 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZMI32WPPEW62XJSBJYWOTUFWGH5ANCNFSM5FMK7NKA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

darioizzo · 2022-04-22T08:56:49Z

@tarcisiofischer
Sorry for this delay .... this is good to go ... would you be willing to implement the same fix for the pso_gen? If you have not the time now we can also merge this in the meantime ....

tarcisiofischer · 2022-05-05T21:38:10Z

@tarcisiofischer Sorry for this delay .... this is good to go ... would you be willing to implement the same fix for the pso_gen? If you have not the time now we can also merge this in the meantime ....

Don't worry about it :)
Unfortunately, I moved away from the project that was using this :( I may be able to implement the same fix eventually, but now is not a good time for me, sorry. I suggest merging this as it is, I can get back to it when I have the time to do so, if that's ok.

Tarcisio Fischer added 2 commits October 5, 2021 15:30

Keeping PSO variables at memory so that consecutive calls to evolve a…

d9eb8ee

…re equal

Refactoring: Move memory data to specific struct

e3b6d08

tarcisiofischer commented Oct 5, 2021

View reviewed changes

src/algorithms/pso.cpp Show resolved Hide resolved

bluescarni requested changes Oct 6, 2021

View reviewed changes

Add PSO memory object serialization

003f435

Code Review: Replace population size type

e74abb3

tarcisiofischer commented Oct 6, 2021

View reviewed changes

include/pagmo/algorithms/pso.hpp Outdated Show resolved Hide resolved

bluescarni requested changes Oct 6, 2021

View reviewed changes

include/pagmo/algorithms/pso.hpp Outdated Show resolved Hide resolved

include/pagmo/algorithms/pso.hpp Outdated Show resolved Hide resolved

include/pagmo/algorithms/pso.hpp Outdated Show resolved Hide resolved

Tarcisio Fischer added 2 commits October 7, 2021 08:24

Code Review: Header cleanup and build fixes

c4ef9bf

Code Review: Move m_V to memory

091c1f4

tarcisiofischer commented Oct 7, 2021

View reviewed changes

Replace broken paper link for genetic operators

462e1c5

Merge branch 'master' into fb-fix-pso-memory

9daa54f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fb fix pso memory #487

Fb fix pso memory #487

tarcisiofischer commented Oct 5, 2021

bluescarni Oct 6, 2021

tarcisiofischer Oct 6, 2021

bluescarni commented Oct 6, 2021

tarcisiofischer commented Oct 6, 2021 •

edited

bluescarni commented Oct 6, 2021

bluescarni commented Oct 6, 2021

bluescarni commented Oct 6, 2021

tarcisiofischer Oct 7, 2021

bluescarni Oct 7, 2021

tarcisiofischer commented Oct 7, 2021

bluescarni commented Oct 7, 2021

bluescarni commented Oct 7, 2021

bluescarni commented Oct 7, 2021

darioizzo commented Oct 7, 2021 via email

darioizzo commented Apr 22, 2022

tarcisiofischer commented May 5, 2022

Fb fix pso memory #487

Are you sure you want to change the base?

Fb fix pso memory #487

Conversation

tarcisiofischer commented Oct 5, 2021

bluescarni Oct 6, 2021

Choose a reason for hiding this comment

tarcisiofischer Oct 6, 2021

Choose a reason for hiding this comment

bluescarni commented Oct 6, 2021

tarcisiofischer commented Oct 6, 2021 • edited

bluescarni commented Oct 6, 2021

bluescarni commented Oct 6, 2021

bluescarni commented Oct 6, 2021

tarcisiofischer Oct 7, 2021

Choose a reason for hiding this comment

bluescarni Oct 7, 2021

Choose a reason for hiding this comment

tarcisiofischer commented Oct 7, 2021

bluescarni commented Oct 7, 2021

bluescarni commented Oct 7, 2021

bluescarni commented Oct 7, 2021

darioizzo commented Oct 7, 2021 via email

darioizzo commented Apr 22, 2022

tarcisiofischer commented May 5, 2022

tarcisiofischer commented Oct 6, 2021 •

edited