-
Notifications
You must be signed in to change notification settings - Fork 162
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
Fb fix pso memory #487
base: master
Are you sure you want to change the base?
Fb fix pso memory #487
Conversation
include/pagmo/algorithms/pso.hpp
Outdated
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this to boost::optional
, so that I can use #include <boost/serialization/optional.hpp>
. Worked as expected :)
@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 I was also thinking that perhaps as an alternative to the |
Nice thanks @bluescarni :)
I've implemented the serialization support. It was really straightforward, but perhaps I did missed something. Please take a look!
Hmm... Are you concerned about the |
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).
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 :) |
The docs build failure is due to a web link that became dead. I'll fix it later. |
The other two build failures look genuine. Could you fix them? |
@@ -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* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
I can do it, but I need some help here.
Which seems to be automatically generated from 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! |
@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... |
Nope... @tarcisiofischer do you mind replacing the link on the fly? |
@tarcisiofischer cheers and thanks for the PR! I am ok with merging as soon as @darioizzo takes a second look. |
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>.
|
@tarcisiofischer |
Don't worry about it :) |
Related to #486