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

SeqAn2 version check #748

Closed
mariehoffmann opened this issue Feb 18, 2019 · 8 comments · Fixed by #955
Closed

SeqAn2 version check #748

mariehoffmann opened this issue Feb 18, 2019 · 8 comments · Fixed by #955

Comments

@mariehoffmann
Copy link
Contributor

Add seqan2 version check in build_system/seqan3-config.cmake. Some headers in seqan3 include seqan2 headers. When using seqan 2.2, you run into a bug (ambiguous usage of empty()), reported also here: rrwick/Porechop#76. Related bug was fixed somewhere between 2.2.0 and 2.4.0. With 2.4 you are on the save side.

@rrahn
Copy link
Contributor

rrahn commented Feb 18, 2019

In seqan3 we don't include seqan2. It is only for the performance builds where we compare against seqan2 to evaluate possible performance regressions in some of the benchmarks. These tests only check if SeqAn was installed (can be found in the PATH environment or in the system's include paths). We could instead have the cmake file in the performance section try to detect seqan and require the version 2.4 there.

@smehringer
Copy link
Member

We could instead have the cmake file in the performance section try to detect seqan and require the version 2.4 there.

sounds good

@h-2
Copy link
Member

h-2 commented Feb 19, 2019

I think this is all a bit much. I don't want to clutter seqan3's build system with seqan2 stuff. The beauty of header-only libraries in combination with __has_include is that we can do everything from C++ and don't need extra tooling. If you think a version check is necessary you can do it via macro inside the code.
But performance checks are only performed by us and from my POV it is self-evident that if you want this very-optional-not-documented-for-the-devs-of-the-library feature you at least take the latest version!

@rrahn
Copy link
Contributor

rrahn commented Feb 19, 2019

What is a bit much? It is one line in the cmake file of the performance builds. Nothing will be added to the seqan3 build system.

@mariehoffmann
Copy link
Contributor Author

mariehoffmann commented Feb 19, 2019 via email

@mariehoffmann
Copy link
Contributor Author

mariehoffmann commented Feb 19, 2019 via email

@h-2
Copy link
Member

h-2 commented Feb 19, 2019

What is a bit much? It is one line in the cmake file of the performance builds. Nothing will be added to the seqan3 build system.

I am pretty sure it is more than one line, but if you volunteer to do it, don't let me stop you.

I assumed benchmarks are also interesting for app writers who want to figure out which approach out of many is the fastest.

No, I think we should publish clear instructions (possibly with numbers) in the API documentation. App writers definitely should not have to run our benchmarks to figure out which types to use.

How do you infer from an ambiguous usage of 'empty' that seqan2 is not uptodate?

I don't know, maybe we just have a different approach. If I encounter a problem, but I know there are other people using this who apparently don't have this problem, the first thing I do is check if my software is up to date 🤓
(But I am used to broken system software from years of UNIX experience so that might not be representative)

Just to give an example, the anchor_block approach for sequence decoration uses a fixed block width whose optimal value is use-case and platform depending. The benchmark could be used to "learn" the optimal width.

I don't think this is feasible for our app developers. We need to figure out sensible default values and suggest very few, clear alternatives for selected scenarios, e.g The default block size is good for sequence lengths < 10.000. For longer sequences you should increase this value.
In the end app developers need to benchmark their own app that's much more relevant than SeqAn's micro-benchmark which are primarily there to track performance regressions (in our code or compilers).

@marehr
Copy link
Member

marehr commented Feb 20, 2019

I needed to adjust some stuff to make seqan2 iterators usable with ranges and introduced a seqan2 header file (only available for test files) in #755.

@h-2 This would solve this, but I'm not sure if you okay with this approach. If you have some suggestions, I'm happy to include them.

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 a pull request may close this issue.

5 participants