search

anigmetov / hera

Other
4 stars 3 forks source link

Confusing dnn includes #11

Closed mglisse closed 1 year ago

mglisse commented 1 year ago

https://github.com/anigmetov/hera/blob/e03c38c97d0ccfc6c8d8d9ac3c092cd7db70b403/include/hera/wasserstein/basic_defs_ws.h#L49

From the use of "" (not <>) for this inclusion, are we supposed to copy extern/dnn into include/hera/wasserstein/ if we want to install hera? For instance, would you expect the debian/ubuntu package to put it in /usr/include/hera/wasserstein/dnn, or in /usr/include/dnn?

Do you think it would be possible to merge the 2 versions of dnn (one in bottleneck and one in extern)? I am confused why the version from wasserstein moved to extern while the version from bottleneck remained in bottleneck :confused:

anigmetov commented 1 year ago

Sorry, it should be <>, fixed. I did not think about the paths from the installation/package perspective.

Actually, the reason there was this mess is because I wanted to leave only one version of DNN. However, when I got to the bottleneck part, I realized there are significant differences and decided to leave it where it was. Anyway, it looks cleaner to have it all in the extern directory, so I just mechanically put both versions into one place (they are still in different namespaces, except the common part), see last commit.

mrzv commented 1 year ago

@anigmetov But what about Marc's question about how this should be installed? And where.

anigmetov commented 1 year ago

@mrzv @mglisse I am starting to think that I should just move all this stuff into include/hera/extern, so that when you install Hera, the external include files go to /usr/include/hera/extern. All the code that I have in the extern directory now is actually header-only (DNN, PHAT, opts, Catch2). I wouldn't like any of these to be copied outside of the Hera directory on my machine. If you think it is a sane idea, I'll just move the hera/extern to hera/include/hera/extern and update the CMakeLists accordingly.

mrzv commented 1 year ago

@anigmetov For DNN, I think the answer is Yes. (We do this in DIY with include/diy/thirdparty, if you want something to reference.) But it's much less obvious with opts and Catch2. Presumably opts is only used for parsing command-line arguments in examples, and Catch2 is only used in tests. If that's correct, then they don't belong in include/hera/extern and shouldn't be installed. I don't know how PHAT is used, so no comment there.

mglisse commented 1 year ago

Having DNN somewhere under /usr/include/hera makes sense to me (I wouldn't even bother with extern and have just include/hera/dnn, but then I don't know this code...), especially if you write the #include lines in a way that I don't need any extra -I flag to find them (maybe #include "../extern/dnn/whatever", unless it is #include <hera/extern/dnn/whatever>).

anigmetov commented 1 year ago

OK, if understand correctly, #include <hera/dnn/whatever> should not cause any trouble, so that's what I did, after moving the dnn code into include/hera. As Dmitriy pointed out, other parts of code in extern don't have to be installed at all.

mglisse commented 1 year ago

Thank you. As far as I am concerned, this fixed this issue.