-
Notifications
You must be signed in to change notification settings - Fork 113
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
Add header files to support independent compilation of volesti headers #212
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #212 +/- ##
========================================
Coverage 55.24% 55.24%
========================================
Files 85 85
Lines 4802 4802
Branches 2108 2108
========================================
Hits 2653 2653
Misses 896 896
Partials 1253 1253
|
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.
Thanks for this PR.
Did you check that now the header files compile independently? Adding toy examples that include headers files of the library and compile independently would be helpful.
Yes some examples are running correctly but i got an error while building This error will be resolved my including All the tests and build ran successfully after making that change.Now the examples compile independently without explicitly including So should i make the changes and put a commit for the same ? |
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.
Thanks! Some thoughts here.
-
I would suggest not including
bits/stdc++.h
since it is not standard and will include a lot of unnecessary headers. -
Also your solution currently partially solves the issue; see what happens if one tries to include
boundary_rdhr_walk.hpp
instead ofrandom_walks.hpp
. Similar, issues are expected with some other walks too e.g.boltzmann_hmc_walk.hpp
.
Do i need to check importing all walks files in example and try compiling them independent of header files or this much changes are enough? Kindly suggest me if any changes are to be done. Thanks! |
Could you please check all files in |
Yes sure i will check for all of them. I have tried compiling It says |
This issue is resolved by including |
OK, please add a comment on the code on why you included that header file. What about all other header files? Are they OK? |
Hi @vissarion, I think this check should be automated so that this is ensured in every PR. |
Yes sure i will look into this answer.
Hello @vissarion I have added all the necessary header files in all the files of All the files under |
Hi @Neel-Shah-29 thanks. Would you like to try the automated procedure proposed by @vaithak ? |
Yes i can try that approach as well, but can this PR be merged first and then i try that approach, as issue #184 can be resolved by this PR. I will submit another PR for that approach,is that ok? |
Hi @Neel-Shah-29, I would prefer that the automated check be done in this PR only, reason being that whenever a bug is solved, the corresponding PR should add some form of check/tests ensuring that this bug won't happen again in the future. This check will ensure that, also this will be make it easy for the reviewers to verify that the problem is actually solved for all the header files. |
Hello @vaithak i have pushed the code after adding the custom test kindly check whether its ok or do i need to make any changes. After running |
Hi @Neel-Shah-29, if you see your log message no target is made for any of the header files. You would have to change the glob expression for searching files.
|
Hello @vaithak this solution worked great , i am getting target to all header files successfully as seen in below screenshot. |
@Neel-Shah-29, I think that is because of only using filenames without extensions for creating the custom targets, you can also include the extension for naming the target. You will have to refer to cmake's documentation on how that can be done. |
Hello @vissarion @vaithak ,This problem was solved by replacing But i got an error regarding |
Hi @Neel-Shah-29. I suspected this would happen as a single header file can include other header files.
This may also require setting WORKING-DIRECTORY of add_custom_target as we are using relative paths with -I |
Hi @vaithak The solution given by you worked fine for me , i have added the |
The issue is same here, you have to provide the include directories for these header files. |
Hello @vaithak , I have made the necessary changes but since the |
You can use CMAKE_SOURCE_DIR variable to set the absolute path of working directory. |
@@ -12,7 +12,7 @@ | |||
|
|||
#include <boost/math/distributions/students_t.hpp> | |||
#include <boost/math/special_functions/erf.hpp> | |||
|
|||
#include <volume/sampling_policies.hpp> |
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.
use ""
instead of <>
as non standard header.
@@ -10,7 +10,8 @@ | |||
#define ROTATING_H | |||
|
|||
#include <Eigen/Eigen> | |||
|
|||
#include<boost/random.hpp> |
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.
for proper formatting, ensure space between #include
and <
@@ -10,7 +10,7 @@ | |||
|
|||
#ifndef SAMPLERS_SPHERE_HPP | |||
#define SAMPLERS_SPHERE_HPP | |||
|
|||
#include<cmath> |
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.
same here, spacing.
@@ -2,7 +2,8 @@ | |||
#define GAUSSIAN_HELPERS_HPP | |||
|
|||
#define EXP_CHORD_TOLERENCE 0.00000001 | |||
|
|||
#include<algorithm> | |||
#include<cmath> |
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.
spacing
#include<vector> | ||
#include<Eigen/Eigen> | ||
#include<utility> | ||
#include<iostream> |
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.
spacing
@@ -10,6 +10,7 @@ | |||
|
|||
#ifndef ODE_SOLVERS_LEAPFROG_HPP | |||
#define ODE_SOLVERS_LEAPFROG_HPP | |||
#include "Eigen/Eigen" |
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.
everywhere in the code, we have used <>
for eigen and boost, so I think we should keep it consistent.
@@ -12,7 +12,7 @@ | |||
|
|||
#ifndef ODE_SOLVERS_RANDOMIZED_MIDPOINT_HPP | |||
#define ODE_SOLVERS_RANDOMIZED_MIDPOINT_HPP | |||
|
|||
#include "Eigen/Eigen" |
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.
same here.
Hey @vaithak I have made the above changes on my local computer , I will be pushing a commit in a while |
It seems that this PR is stale for a long time. I marked it |
This PR fixes #184
According to the issue , The
cmath
header file was not included inboundary_cdhr_walk.hpp
, which was giving errors of pow , sqrt functions. So i have made the necessary changes. Kindly suggest if any other changes needs to be done.