pierreroudier commented 3 years ago

[x] Rename the extract options to more explicit values (issue https://github.com/pierreroudier/opusreader/issues/2)
[x] Make a call regarding the simplify option (issue https://github.com/pierreroudier/opusreader/issues/3). We could either:
- keep it as is
- keep it as is, and add a warning regarding the approximation of wavenumbers, and advise to revert to simplify = FALSE if the user wants exact wavenumbers
- implement a simple linear interpolation to get values at rounded wavenumbers
- discard it completely
[x] Add tests
[x] Improve documentation, in particular document the list returned when simplify = FALSE

@philipp-baumann feel free to add anything you think might be required before CRAN submission.

pierreroudier commented 3 years ago

Update: added tests in https://github.com/pierreroudier/opusreader/commit/585c43aeb649b1fd071c0d16fe7a442743c3847c

pierreroudier commented 3 years ago

Update: implemented simple linear interpolation to get values at rounded wavenumbers in https://github.com/pierreroudier/opusreader/commit/3ad55474fde2f10aff26a9d210baadfac4184a03

pierreroudier commented 3 years ago

Update: The case of simplify was addressed by https://github.com/pierreroudier/opusreader/commit/3ad55474fde2f10aff26a9d210baadfac4184a03 and PR https://github.com/pierreroudier/opusreader/pull/5

pierreroudier commented 3 years ago

Update: The documentation of the output was updated in https://github.com/pierreroudier/opusreader/commit/70bd2c0940df4aa59ce74c7d79e6d0f8731aaefb and https://github.com/pierreroudier/opusreader/commit/e42453476b685608172ef3b833d5547ef5170876

@philipp-baumann could you please review?

philipp-baumann commented 3 years ago

Update: The documentation of the output was updated in 70bd2c0 and e424534

@philipp-baumann could you please review?

@pierreroudier Shall I make a new pull request to review your commit?

pierreroudier commented 3 years ago

@philipp-baumann Sure -- love the PRs :)

Discussion in https://github.com/pierreroudier/opusreader/issues/2 will help refine the docs too.

philipp-baumann commented 3 years ago

New suggestions

[ ] Implement reading of files in parallel chunks (most universally via future; e.g. future.apply::future_lapply()). If there are 100s or 1000s of files, the process of reading could for example be distributed over multiple cores to speed up the process. Key points (let's discuss):
1. Reading single files per core at a given time has too much overhead compared to time it takes to read one file.
2. Chunking could be done by splitting up the list of files into multiple groups according to numbers of cores registered (user).
3. Each chunk of files is read separately and then recombined at the end.

pierreroudier commented 3 years ago

* [ ]  Implement reading of files in parallel chunks (most universally via future; e.g. `future.apply::future_lapply()`).

Agree this would be a valuable addition. with future.apply, the implementation burden is also very low, which is great.

However, I'd be keen to do our first CRAN submit with a package a simple as possible (the initial CRAN submission is always much harder than the subsequent ones in my experience), if we can hold off for a couple weeks.

@philipp-baumann I have created a dedicated issue/enhancement request to track progress on this: https://github.com/pierreroudier/opusreader/issues/8

philipp-baumann commented 3 years ago

Hi Pierre, yes that's fine with me to wait until first CRAN release is scheduled. I might play around with some stuff in some devel branch in my fork. Will let you know.

philipp-baumann commented 3 years ago

This one as a suggestion before our first CRAN submission

Versioning in NEWS.md #9

pierreroudier commented 3 years ago

This one as a suggestion before our first CRAN submission
* Versioning in `NEWS.md` #9

Done in https://github.com/pierreroudier/opusreader/commit/2f0014faf04a430be493fa49427da1efd0d06eeb. Thanks for pointing it out, it's an awesome tool!

pierreroudier commented 3 years ago

Issue https://github.com/pierreroudier/opusreader/issues/2 has been closed, which is ticking off the last item remaining before attempting our first CRAN release!

Please review. I will update the dev version number to reflect change, and update NEWS.md in the meantime.

pierreroudier commented 3 years ago

@philipp-baumann I have been slowly getting back into opusreader. It seems we are much closer to an initial CRAN release,are you happy for me to initiate packaging? As it is, this would be a version 0.4.2, so not as "committing" as a 1.0 :)

pierreroudier / opusreader

Initial CRAN release checklist #4

New suggestions