[FEATURE] Serialise index cursor #2048

eseiler · 2020-08-18T11:04:11Z

Resolves #2044

marehr · 2020-08-18T19:27:12Z

Header test failed:

In file included from /home/travis/build/seqan/seqan3-build/seqan3_header_test_files/seqan3/search/fm_index/detail/fm_index_cursor-1.cpp:2:0:

/home/travis/build/seqan/seqan3/include/seqan3/search/fm_index/detail/fm_index_cursor.hpp:72:15: error: ‘cereal_archive’ has not been declared
     template <cereal_archive archive_t>
               ^~~~~~~~~~~~~~
/home/travis/build/seqan/seqan3/include/seqan3/search/fm_index/detail/fm_index_cursor.hpp:73:41: error: ‘archive_t’ has not been declared
     void CEREAL_SERIALIZE_FUNCTION_NAME(archive_t & archive)
                                         ^~~~~~~~~
make[2]: *** [CMakeFiles/seqan3_header_test--seqan3-searc

(An include is missing for seqan3::cereal_archive)

codecov · 2020-08-19T03:12:49Z

Codecov Report

Merging #2048 into master will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2048      +/-   ##
==========================================
- Coverage   97.93%   97.89%   -0.04%     
==========================================
  Files         267      263       -4     
  Lines        9985     9899      -86     
==========================================
- Hits         9779     9691      -88     
- Misses        206      208       +2

Impacted Files	Coverage Δ
...lude/seqan3/search/fm_index/bi_fm_index_cursor.hpp	`100.00% <100.00%> (ø)`
.../seqan3/search/fm_index/detail/fm_index_cursor.hpp	`100.00% <100.00%> (ø)`
include/seqan3/search/fm_index/fm_index_cursor.hpp	`100.00% <100.00%> (ø)`
.../seqan3/alignment/pairwise/alignment_algorithm.hpp	`98.50% <0.00%> (-1.50%)`	⬇️
include/seqan3/alignment/scoring/gap_scheme.hpp	`96.29% <0.00%> (-0.48%)`	⬇️
...qan3/alignment/pairwise/edit_distance_unbanded.hpp	`99.24% <0.00%> (-0.01%)`	⬇️
include/seqan3/core/detail/to_string.hpp	`100.00% <0.00%> (ø)`
include/seqan3/search/configuration/hit.hpp	`100.00% <0.00%> (ø)`
test/include/seqan3/test/pretty_printing.hpp	`100.00% <0.00%> (ø)`
include/seqan3/io/alignment_file/format_bam.hpp	`95.49% <0.00%> (ø)`
... and 23 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 e56b34b...7f46653. Read the comment docs.

smehringer

Minor stuff

smehringer · 2020-08-19T06:08:36Z

include/seqan3/search/fm_index/bi_fm_index_cursor.hpp

+
+    /*!\cond DEV
+     * \brief Serialisation support function.
+     * \tparam archive_t Type of `archive`; must satisfy seqan3::cereal_archive.


Suggested change

* \tparam archive_t Type of `archive`; must satisfy seqan3::cereal_archive.

* \tparam archive_t Type of `archive`; must model seqan3::cereal_archive.

💅

We use satifsy in every serialisation method (~12 times). Can I do this as separated PR?

smehringer · 2020-08-19T06:09:26Z

include/seqan3/search/fm_index/detail/fm_index_cursor.hpp

+
+    /*!\cond DEV
+     * \brief Serialisation support function.
+     * \tparam archive_t Type of `archive`; must satisfy seqan3::cereal_archive.


Suggested change

* \tparam archive_t Type of `archive`; must satisfy seqan3::cereal_archive.

* \tparam archive_t Type of `archive`; must model seqan3::cereal_archive.

smehringer · 2020-08-19T06:09:40Z

include/seqan3/search/fm_index/fm_index_cursor.hpp

+
+    /*!\cond DEV
+     * \brief Serialisation support function.
+     * \tparam archive_t Type of `archive`; must satisfy seqan3::cereal_archive.


Suggested change

* \tparam archive_t Type of `archive`; must satisfy seqan3::cereal_archive.

* \tparam archive_t Type of `archive`; must model seqan3::cereal_archive.

smehringer · 2020-08-19T06:12:44Z

include/seqan3/search/fm_index/bi_fm_index_cursor.hpp

+     */
+    template <cereal_archive archive_t>
+    void CEREAL_SERIALIZE_FUNCTION_NAME(archive_t & archive)
+    {


Since this function is not visible to the user, we should mention this in the class documentation. I think we might just used \imlements cerealisable or something similar.. Can you check other classes?

Also Can you add a changelog entry, since this seems to be of interest to users :)

Yes, but then I would also rebase this against the release branch?

Since this function is not visible to the user, we should mention this in the class documentation. I think we might just used \imlements cerealisable or something similar.. Can you check other classes?

Yes, it would be great to see directly in the documentation which classes can be serialized with cereal, I had to study the code to see that neither cerealisation is possible, nor I can access the members on my own. I guess adding something like \implements cerealisable would be perfectly suitable for this.

Currently the documentation is only reflected in the inheritance diagram as far as I can see, https://docs.seqan.de/seqan/3-master-user/interfaceseqan3_1_1cerealisable.html

We should rework this

eseiler · 2020-08-19T09:07:21Z

I'll squash and push in the hope that travis notices something

smehringer · 2020-08-31T16:19:56Z

I forgot about it again 🙈 Sorry

eseiler mentioned this pull request Aug 18, 2020

FM index cursor (de-)serialization #2044

Closed

eseiler requested review from a team and marehr and removed request for a team August 18, 2020 11:09

marehr approved these changes Aug 18, 2020

View reviewed changes

marehr requested review from a team and smehringer and removed request for a team August 18, 2020 12:19

eseiler force-pushed the feature/serialise_cursor branch from 96e7e33 to c762759 Compare August 18, 2020 20:07

smehringer requested changes Aug 19, 2020

View reviewed changes

eseiler force-pushed the feature/serialise_cursor branch from c762759 to 8032f8c Compare August 19, 2020 07:46

eseiler changed the base branch from master to release-3.0.2 August 19, 2020 07:46

eseiler requested a review from smehringer August 19, 2020 07:47

smehringer approved these changes Aug 19, 2020

View reviewed changes

eseiler force-pushed the feature/serialise_cursor branch from 8032f8c to e5bb2bb Compare August 19, 2020 09:07

tloka mentioned this pull request Aug 19, 2020

FM index cursor enable node access #2061

Closed

[FEATURE] Serialise index cursor

7f46653

eseiler changed the base branch from release-3.0.2 to master August 25, 2020 15:07

eseiler force-pushed the feature/serialise_cursor branch from e5bb2bb to 7f46653 Compare August 25, 2020 15:07

eseiler requested a review from smehringer August 31, 2020 11:32

smehringer approved these changes Aug 31, 2020

View reviewed changes

smehringer merged commit 9babc11 into seqan:master Aug 31, 2020

eseiler deleted the feature/serialise_cursor branch November 17, 2020 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Serialise index cursor #2048

[FEATURE] Serialise index cursor #2048

eseiler commented Aug 18, 2020

marehr commented Aug 18, 2020

codecov bot commented Aug 19, 2020 •

edited

Loading

smehringer left a comment

smehringer Aug 19, 2020

eseiler Aug 19, 2020

smehringer Aug 19, 2020

smehringer Aug 19, 2020

smehringer Aug 19, 2020

smehringer Aug 19, 2020

eseiler Aug 19, 2020

tloka Aug 19, 2020

eseiler Aug 19, 2020

eseiler commented Aug 19, 2020

smehringer commented Aug 31, 2020

	* \tparam archive_t Type of `archive`; must satisfy seqan3::cereal_archive.
	* \tparam archive_t Type of `archive`; must model seqan3::cereal_archive.

[FEATURE] Serialise index cursor #2048

[FEATURE] Serialise index cursor #2048

Conversation

eseiler commented Aug 18, 2020

marehr commented Aug 18, 2020

codecov bot commented Aug 19, 2020 • edited Loading

Codecov Report

smehringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseiler commented Aug 19, 2020

smehringer commented Aug 31, 2020

codecov bot commented Aug 19, 2020 •

edited

Loading