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

[INFRA] Move core/algorithm into core/configuration #2255

Conversation

Irallia
Copy link
Contributor

@Irallia Irallia commented Nov 11, 2020

Resolves partly seqan/product_backlog#160

algorithm/concept.hpp -> core/configuration/detail/.
algorithm/configuration.hpp -> core/configuration/.
algorithm/configuration_element_debug_mode.hpp -> core/configuration/detail/.
algorithm/configuration_element_parallel_mode.hpp -> core/configuration/detail/.
algorithm/configuration_utility.hpp -> core/configuration/detail/.

Skipped: algorithm/pipeable_config_element.hpp (needs a decision)

Keep untouched:
algorithm/detail/... -> core/algorithm/detail/...
algorithm/algorithm_result_generator_range -> core/algorithm/algorithm_result_generator_range

Added new all.hpp's and adjusted the other ones.

Details for this: https://gist.github.com/rrahn/f6b09ad67bebd14bd9c58f77c53e0450

@Irallia Irallia self-assigned this Nov 11, 2020
@Irallia Irallia requested review from a team and joergi-w and removed request for a team November 11, 2020 15:03
@codecov
Copy link

codecov bot commented Nov 11, 2020

Codecov Report

Merging #2255 (1ae6a3f) into master (d37ba83) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2255   +/-   ##
=======================================
  Coverage   98.15%   98.15%           
=======================================
  Files         262      262           
  Lines       10815    10815           
=======================================
  Hits        10616    10616           
  Misses        199      199           
Impacted Files Coverage Δ
...ude/seqan3/alignment/pairwise/alignment_result.hpp 100.00% <ø> (ø)
...gnment/pairwise/detail/policy_alignment_matrix.hpp 100.00% <ø> (ø)
...airwise/detail/policy_alignment_result_builder.hpp 100.00% <ø> (ø)
...ignment/pairwise/detail/policy_optimum_tracker.hpp 97.82% <ø> (ø)
...e/seqan3/alignment/pairwise/detail/type_traits.hpp 100.00% <ø> (ø)
...qan3/alignment/pairwise/edit_distance_unbanded.hpp 99.31% <ø> (ø)
...ignment/pairwise/policy/simd_affine_gap_policy.hpp 100.00% <ø> (ø)
...an3/search/configuration/default_configuration.hpp 100.00% <ø> (ø)
include/seqan3/search/search.hpp 100.00% <ø> (ø)
include/seqan3/search/search_result.hpp 100.00% <ø> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d37ba83...1ae6a3f. Read the comment docs.

@Irallia Irallia requested review from joergi-w and removed request for joergi-w November 12, 2020 10:35
@Irallia Irallia force-pushed the INFRA/core/move_header_algorithm_to_configuration branch 5 times, most recently from a0f7a83 to 0befa64 Compare November 16, 2020 10:29
include/seqan3/core/algorithm/all.hpp Outdated Show resolved Hide resolved
include/seqan3/core/configuration/detail/all.hpp Outdated Show resolved Hide resolved
include/seqan3/core/algorithm/concept.hpp Outdated Show resolved Hide resolved
include/seqan3/core/configuration/configuration.hpp Outdated Show resolved Hide resolved
include/seqan3/core/algorithm/all.hpp Outdated Show resolved Hide resolved
include/seqan3/core/algorithm/detail/all.hpp Outdated Show resolved Hide resolved
include/seqan3/core/configuration/all.hpp Show resolved Hide resolved
include/seqan3/core/configuration/detail/concept.hpp Outdated Show resolved Hide resolved
@Irallia Irallia requested a review from joergi-w November 16, 2020 13:47
Copy link
Member

@joergi-w joergi-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@joergi-w joergi-w requested a review from rrahn November 16, 2020 13:53
Copy link
Contributor

@rrahn rrahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not remove the old seqan3/alignment/band in this PR. This has nothing to do with the change of the configuration header. Second the static band should be deprecated before it is removed in 3.1. Can you first revert these changes and then I look into the rest. Thank you

@Irallia Irallia force-pushed the INFRA/core/move_header_algorithm_to_configuration branch 2 times, most recently from 64f920c to 52e071c Compare November 19, 2020 13:22
@Irallia Irallia requested a review from rrahn November 19, 2020 15:33
#include <seqan3/core/algorithm/configuration_utility.hpp>
#include <seqan3/core/algorithm/configuration.hpp>
#include <seqan3/core/detail/pack_algorithm.hpp>
#include <seqan3/core/algorithm/detail/all.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove

#include <seqan3/core/algorithm/pipeable_config_element.hpp>
#include <seqan3/core/configuration/all.hpp> // ToDo: remove this with the release SeqAn-3.1.0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to handle this. This is likely to be forgotten.
I am thinking of this:

#if SEQAN3_VERSION_MAJOR == 3 && SEQAN3_VERSION_MINOR == 1
  #pragma warning "Remove #include <seqan3/core/configuration/all.hpp> from this header."
#endif

@marehr what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was exactly the problem I tried to explain on Gitter on friday..

* \author Rene Rahn <rene.rahn AT fu-berlin.de>
* \deprecated This header will be removed in 3.1. Please use seqan3/core/configuration/detail/concept.hpp instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* \deprecated This header will be removed in 3.1. Please use seqan3/core/configuration/detail/concept.hpp instead.
* \deprecated This header will be removed in 3.1.0; Please \#include <seqan3/core/configuration/detail/concept.hpp> instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed Marcel's example, which he created for us. (math) Now I had to make a lot of changes that did not match his example. Maybe next time you can look over the example as the core team before we take over all the mistakes there.

Comment on lines 11 to 12
* \deprecated This header will be removed in 3.1. Please use
* seqan3/core/configuration/detail/configuration_element_debug_mode.hpp instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* \deprecated This header will be removed in 3.1. Please use
* seqan3/core/configuration/detail/configuration_element_debug_mode.hpp instead.
* \deprecated This header will be removed in 3.1.0; Please \#include <seqan3/core/configuration/detail/configuration_element_debug_mode.hpp> instead.

Comment on lines 20 to 21
"This header is deprecated and will be removed in SeqAn-3.1.0; Please #include "
"<seqan3/core/configuration/detail/configuration_element_debug_mode.hpp> instead.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"This header is deprecated and will be removed in SeqAn-3.1.0; Please #include "
"<seqan3/core/configuration/detail/configuration_element_debug_mode.hpp> instead.")
"This header is deprecated and will be removed in SeqAn-3.1.0; Please #include <seqan3/core/configuration/detail/configuration_element_debug_mode.hpp> instead.")

We said that it is ok to not wrap this around. I think it causes some displaying issues.

Comment on lines 12 to 13
* \deprecated This header will be removed in 3.1. Please use
* seqan3/core/configuration/detail/configuration_element_parallel_mode.hpp instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* \deprecated This header will be removed in 3.1. Please use
* seqan3/core/configuration/detail/configuration_element_parallel_mode.hpp instead.
* \deprecated This header will be removed in 3.1.0; Please \#include
* <seqan3/core/configuration/detail/configuration_element_parallel_mode.hpp> instead.

Comment on lines 21 to 22
"This header is deprecated and will be removed in SeqAn-3.1.0; Please #include "
"<seqan3/core/configuration/detail/configuration_element_parallel_mode.hpp> instead.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"This header is deprecated and will be removed in SeqAn-3.1.0; Please #include "
"<seqan3/core/configuration/detail/configuration_element_parallel_mode.hpp> instead.")
"This header is deprecated and will be removed in SeqAn-3.1.0; Please #include <seqan3/core/configuration/detail/configuration_element_parallel_mode.hpp> instead.")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is your requested change here, that you want everything in one line, even if there are more than 120 characters?

Comment on lines 11 to 12
* \deprecated This header will be removed in 3.1. Please use seqan3/core/configuration/detail/configuration_utility.hpp
* instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* \deprecated This header will be removed in 3.1. Please use seqan3/core/configuration/detail/configuration_utility.hpp
* instead.
* \deprecated This header will be removed in 3.1.0; Please \#include <seqan3/core/configuration/detail/configuration_utility.hpp>
* instead.

Comment on lines 20 to 21
"This header is deprecated and will be removed in SeqAn-3.1.0; Please #include "
"<seqan3/core/configuration/detail/configuration_utility.hpp> instead.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"This header is deprecated and will be removed in SeqAn-3.1.0; Please #include "
"<seqan3/core/configuration/detail/configuration_utility.hpp> instead.")
"This header is deprecated and will be removed in SeqAn-3.1.0; Please #include <seqan3/core/configuration/detail/configuration_utility.hpp> instead.")

@@ -13,4 +13,5 @@
#pragma once

#include <seqan3/core/algorithm/detail/algorithm_executor_blocking.hpp>
#include <seqan3/core/algorithm/detail/execution_handler_parallel.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general we don't want detail to be included inside of the all.hpp.
It basically is no functionality for the user and should be included through the public headers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we leave it for now but I would remove it later. Lets discuss this today in the group meeting

@Irallia
Copy link
Contributor Author

Irallia commented Nov 23, 2020

You might want to check my changes before I rebase.

@Irallia Irallia requested a review from rrahn November 23, 2020 15:29
Copy link
Contributor

@rrahn rrahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK one small thing. I couldn't find the header that should be removed later? The warning is only a message emitted by the compiler. The header include should still exist. Or did I miss it? You can rebase right away.

#include <seqan3/core/algorithm/pipeable_config_element.hpp>
#if SEQAN3_VERSION_MAJOR == 3 && SEQAN3_VERSION_MINOR == 1
#pragma warning "Remove #include <seqan3/core/configuration/all.hpp> from this header."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but where is the header?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

working without the head... 😅

@Irallia Irallia force-pushed the INFRA/core/move_header_algorithm_to_configuration branch from 8e11176 to 8f7ac72 Compare November 25, 2020 15:36
@Irallia Irallia requested a review from rrahn November 25, 2020 17:11
Copy link
Contributor

@rrahn rrahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some minor include order then it is good to go

#include <seqan3/core/configuration/detail/concept.hpp>
#include <seqan3/core/concept/tuple.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include order

algorithm/concept.hpp -> core/configuration/detail/.
algorithm/configuration.hpp -> core/configuration/.
algorithm/configuration_element_debug_mode.hpp -> core/configuration/detail/.
algorithm/configuration_element_parallel_mode.hpp -> core/configuration/detail/.
algorithm/configuration_utility.hpp -> core/configuration/detail/.

Skipped: algorithm/pipeable_config_element.hpp (needs a decision)

Keep untouched:
algorithm/detail/... -> core/algorithm/detail/...
algorithm/algorithm_result_generator_range -> core/algorithm/algorithm_result_generator_range

Added new all.hpp's and adjusted the other ones.

Signed-off-by: Lydia Buntrock <[email protected]>
@Irallia Irallia force-pushed the INFRA/core/move_header_algorithm_to_configuration branch from 8f7ac72 to 1ae6a3f Compare November 30, 2020 12:48
@Irallia Irallia requested a review from rrahn November 30, 2020 13:55
Copy link
Contributor

@rrahn rrahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@rrahn rrahn merged commit f66fe5b into seqan:master Dec 5, 2020
marehr added a commit to marehr/seqan3 that referenced this pull request Dec 22, 2020
PR seqan#2255 did the move from core/algorithm into core/configuration, but at the same time PR seqan#2247 changed one header.

This seems like a misinterpreted merge conflict and this PR reintroduces the changes from seqan#2255. After checking the changes of seqan#2255, this seems to be the only missed change.

This partially reverts commit 1ae6a3f.
marehr added a commit to marehr/seqan3 that referenced this pull request Dec 22, 2020
PR seqan#2255 did the move from core/algorithm into core/configuration, but at the same time PR seqan#2247 changed one header.

This seems like a misinterpreted merge conflict and this PR reintroduces the changes from seqan#2255. After checking the changes of seqan#2255, this seems to be the only missed change.

This partially reverts commit 1ae6a3f.
@Irallia Irallia deleted the INFRA/core/move_header_algorithm_to_configuration branch January 28, 2022 13:42
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 this pull request may close these issues.

5 participants