pagmo2
pagmo2 copied to clipboard
esa
Fb fix pso memory
@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 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
memorystruct serializable and then ensure that the newpsodata memberstd::/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
optionalapproach, you could usestd::unique_ptrinstead. This way the definition of thememorystruct could go in thepso.cppfile 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!)
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
memorystruct in thehppfile due to possible increase of compile time? Can you confirm I should go for it and remove theoptional? (I ask because I prefer theoptionalsolution, 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 :)
The docs build failure is due to a web link that became dead. I'll fix it later.
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!
@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...
@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 https://github.com/esa/pagmo2/pull/487#issuecomment-937740049, 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 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 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.