-
Notifications
You must be signed in to change notification settings - Fork 12
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
Use Conan for installing dependencies #302
Conversation
657fd7e
to
d0f51a8
Compare
|
We need to close astro-informatics/sopt#279 before we can make progress with this |
- How do we build the sopt package automatically? Right now it points to my private test version of the package. - tests/measurement_operator.cc had some lines with semicolons missing! - in tests/mpi_algo_factory.cc I had to comment out statements with CAPTURE(uv_data.vis.head(5)); because something was going wrong with the macro expansion. - the mpi_algo_factory test only passes when the number of processes is 2. Otherwise it seems to hang indefinitely. There is a lot of code around world.size == 2.
…into sj/conan-build
Following the pattern of other flags in this file; PURIFY_CPPFLOW doens't get set properly without this line, I think it is otherwise not defined.
Looking at things here I think that this is up to date now with what I expect, keeping the forward-backward tensorflow test out of it for a separate PR. 👍 |
Ok, from my point of view the only outstanding item is updating the documentation workflow. I think everything else looks good to go. |
Make tests in alphabetical order
Co-authored-by: S Jaffa <[email protected]>
…/purify into sj/conan-build
I've checked the installation instructions in the readme work for me from a clean setup, I've updated the documentation workflow (although not tested this as it only runs when we push to development branch) and the other couple of outdated bits in the readme I've made new issues as they can be solved separately. So if someone could check over the docs workflow I think this is finally ready to merge! (assuming the tests pass...) |
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.
C++ code is good for me; as far as the docs action goes it looks okay to me but I don't think it's possible to run it manually so I'm happy for there to be a separate issue if there needs to be a further tweak there rather than extending this PR any further.
All conversations from the review have now been marked resolved and TK has said he's happy to merge.
Starting from the development branch, first issue was the same spdlog error as in sopt
cppflow==on
option is passed to cmakeclass GProximal
to thesopt::algorithm
namespace