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

adds new cpp funs #27

Merged
merged 19 commits into from
Aug 20, 2021
Merged

adds new cpp funs #27

merged 19 commits into from
Aug 20, 2021

Conversation

Nowosad
Copy link
Contributor

@Nowosad Nowosad commented Jul 13, 2021

I will open a new issue soon.

@Nowosad Nowosad mentioned this pull request Jul 13, 2021
@HajkD
Copy link
Member

HajkD commented Jul 20, 2021

Hi Jakub,

Thank you so much for your fantastic work!!! I truly appreciate all your efforts and I will try to perform a thorough code review in the next days so that I can give you some more feedback and incorporate all your new features into philentropy.

You will hear from me soon :)

Many thanks,
Hajk

@HajkD
Copy link
Member

HajkD commented Jul 20, 2021

Btw: I just added a new argument epsilon to the philentropy::distance() and philentropy::KL() functions (see #26).

Would it be possible to add these changes to your pull request?

Your help is very much appreciated!

Many thanks!

Merge remote-tracking branch 'origin/master' into cpp_extend

# Conflicts:
#	src/philentropy_init.c
@Nowosad
Copy link
Contributor Author

Nowosad commented Jul 22, 2021

Hi @HajkD - I will wait for your code review. If you give me the green light, then I will incorporate the epsilon argument, add all of the possible distance measures to single_distance(), and write some documentation.

There is also one other important change to discuss. I have noticed that you use the src/philentropy_init.c file, which I assume you create by hand. If you look at my PR, you will see this file removed, and its content is now in src/RcppExports.cpp. The RcppExports.cpp is created automatically with devtools::document(). Let me know what you think about this approach. If you do not like it, I can go back to the way it was before.

@HajkD HajkD marked this pull request as ready for review August 18, 2021 13:27
@HajkD
Copy link
Member

HajkD commented Aug 18, 2021

Hi Jakub,

Everything looks excellent and we can remove the src/philentropy_init.c file if it continues to pass all rhub checks.

Do you already want to add the rest and then I merge, or shall I merge first and then you add the rest?

Would it also be possible to add the fantastic benchmark and motivation in #28 to the vignettes so that users can swiftly see the advantages of using the new functions?

Many thanks!!!
Hajk

@HajkD
Copy link
Member

HajkD commented Aug 18, 2021

Also feel free to add your changes to NEWS.md for the next release.

@Nowosad
Copy link
Contributor Author

Nowosad commented Aug 18, 2021

Hi Hajk,
I plan to find some time tomorrow to work on this (adding the rest of the metrics, vignette, and update the news file). I will let you know when it is ready to be merged.
Best,
Jakub

@Nowosad
Copy link
Contributor Author

Nowosad commented Aug 20, 2021

Hi Hajk,
I think this PR is ready for your final review (and merge). To sum up:

  1. It adds three new functions - dist_one_one(), dist_one_many(), dist_many_many()
  2. These functions are exported and documented
  3. NEWS are updated
  4. There is also a vignette explaining all of them - Many_Distances.Rmd
  5. It also fixes some existing issues with documentations
  6. Finally, I hide the last part of the existing Distances.Rmd vignette, as it seems unfinished

Best, J

@HajkD
Copy link
Member

HajkD commented Aug 20, 2021

Hi Jakub,

Absolutely brilliant! I already reviewed your changes after you committed and everything looks fantastic. I will now merge everything and we shall run a quick integration test. Do you think it would make sense to add some unit tests with testthat so that we can always run automated checks as we continue to grow the package?

I was also wondering whether you would be interested (and/or have time) to look into the parallel support feature #24? This would also speed up big computations.

In either case, I am more than happy to add you as official contributor to the package and let me know if you have other ideas that we could integrate before submitting the new package version to CRAN.

You wonderful help is greatly appreciated!

Many thanks,
Hajk

@Nowosad
Copy link
Contributor Author

Nowosad commented Aug 20, 2021

Hi Hajk,

  1. new tests are now added - 228e67f
  2. I will try to look at the parallel support some time in the future, but I cannot promise anything (as my C++ skills are just good enough to barely survive;))

@HajkD HajkD merged commit 2c36520 into drostlab:master Aug 20, 2021
@Nowosad Nowosad deleted the cpp_extend branch August 28, 2021 16:47
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.

2 participants