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

Initial CRAN release checklist #4

Open
4 tasks done
pierreroudier opened this issue Apr 28, 2021 · 13 comments
Open
4 tasks done

Initial CRAN release checklist #4

pierreroudier opened this issue Apr 28, 2021 · 13 comments

Comments

@pierreroudier
Copy link
Owner

pierreroudier commented Apr 28, 2021

  • Rename the extract options to more explicit values (issue Refactoring of the extract option #2)
  • Make a call regarding the simplify option (issue Concatenating spectra with the simple option #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
  • Add tests
  • 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
Copy link
Owner Author

Update: added tests in 585c43a

@pierreroudier
Copy link
Owner Author

Update: implemented simple linear interpolation to get values at rounded wavenumbers in 3ad5547

@pierreroudier
Copy link
Owner Author

Update: The case of simplify was addressed by 3ad5547 and PR #5

@pierreroudier
Copy link
Owner Author

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

@philipp-baumann could you please review?

@philipp-baumann
Copy link
Collaborator

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
Copy link
Owner Author

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

Discussion in #2 will help refine the docs too.

@philipp-baumann
Copy link
Collaborator

philipp-baumann commented May 5, 2021

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
Copy link
Owner Author

pierreroudier commented May 5, 2021

* [ ]  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: #8

@philipp-baumann
Copy link
Collaborator

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
Copy link
Collaborator

philipp-baumann commented May 6, 2021

This one as a suggestion before our first CRAN submission

@pierreroudier
Copy link
Owner Author

This one as a suggestion before our first CRAN submission

* Versioning in `NEWS.md` #9

Done in 2f0014f. Thanks for pointing it out, it's an awesome tool!

@pierreroudier
Copy link
Owner Author

Issue #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
Copy link
Owner Author

@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 :)

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

No branches or pull requests

2 participants