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

Fb fix pso memory #487

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

tarcisiofischer
Copy link

Related to #486

include/pagmo/algorithms/pso.hpp Outdated Show resolved Hide resolved
@@ -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;
Copy link
Member

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.

Copy link
Author

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 :)

src/algorithms/pso.cpp Show resolved Hide resolved
@bluescarni
Copy link
Member

@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
Copy link
Author

tarcisiofischer commented Oct 6, 2021

@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 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
@bluescarni
Copy link
Member

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
Copy link
Member

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

@bluescarni
Copy link
Member

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*
Copy link
Author

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.

Copy link
Member

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).

@tarcisiofischer
Copy link
Author

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
Copy link
Member

@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
Copy link
Member

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

@bluescarni
Copy link
Member

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

@darioizzo
Copy link
Member

darioizzo commented Oct 7, 2021 via email

@darioizzo
Copy link
Member

@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
Copy link
Author

@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.

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

Successfully merging this pull request may close these issues.

3 participants