r-hyperspec / hyspc.read.spe Goto Github PK

@sangttruong, please read the documentation of this package (https://r-hyperspec.github.io/hySpc.read.spe/) and tell whether, in your opinion, it is of sufficient quality or not. Please, suggest bellow, how should it be improved. If possible, add improvements as a separate PR. If it is OK, let us know too.

The labels in hyperSpec and this repo are different. We should find a way how to copy them to this repo. It would be perfect if we could automate this process.

Some things that still need to be done to complete the infrastructure.

• Go through CONTRIBUTING.md and change references to SKELETON or skeleton to hySpc.read.spe as appropriate.
• Remove demo_function and unittestdata files.
• Remove project.Proj and also gitignore it.
• Review the to-do list hidden in README.md and check off, complete or ideally delete the whole section because it's all done!
• I (Bryan) don't see any test files in the package... did these get left behind in hyperSpec? There are mentions of them in read_spe unit tests.
• read_spe needs an example, maybe in \dontrun{
• Deprecate functions over in hyperSpec as needed.
• A vignette is missing. Please look in hyperSpec to see if there is material that should be moved to a vignette.
• IF THERE WILL BE A VIGNETTE The vignette needs "front material" that sets up a lot of things. See hyperSpec.Rmd for the needed material. There are also some additional files that need to be copied to this repo (should be clear from hyperSpec.Rmd). After moving the needed material and files, do a basic edit to remove stuff that is clearly not appropriate for this repo. MAYBE EASIER TO GET THE FRONT MATERIAL FROM hySpc.read.jdx.

• Unit tests should be reviewed and, if needed, updated to make sure they work properly.
• If possible, coverage over 90% should be acheved.

The blut1.spe file doesn't work because its ydim = 0, and ydim is used to set the dimention of another matrix (hence the function throws an error). read_spe() works just fine on spe_format_3.0.spe.

with some file in inst/extdata/, provide examples for

• read_spe()
• spe_showcalpoints()

In read_*() functions we currently use file, files, filename or con to indicate path to file or connection. We should use file consistently. Update argument name in these functions:

• read_spe()
• other

It gets a bit confusing, as it is not always clear who should be included as authors, maintainers, and contributors (aut, cre, ctb) of this package (hySpc.read.spe) and other packages that are made of code separated from the hyperSpec, I open this issue for discussion and clarifications.

In my opinion, @cbeleites knows best, who created what in hyperSpec.

@cbeleites, when you have time, please, review the DESCRIPTION file and let us know if the author list is complete.

Check and correct the type of license.

More text and code should be added to the vignette otherwise it should be deleted completely.

Would it be sufficient to add everything to examples? Please, express your point of view and opinion on this @sangttruong, @cbeleites, @bryanhanson

Improve code coverage as much as possible. covr::report() might be useful to find the blind spots (lines that are not covered).

Check and update the author and contributor list on DESCRIPTION.

to find a small subset to be shipped with the package.

into its own .R file

Improve documentation by adding @examples sections to all exported functions. Code possibly should go inside \donttest{\dontrun{ ... }} if there are no other options to make it work correctly.

Is it reasonable to rename argument xaxis to xaxis_units, wl_units or similar?

@cbeleites, what do you think?

UPDATE

Tasks:

• Rename arg to wl_units.
• Deprecate xaxis for backward compatibility.
• Update NEWS.md.

• update file names to refer to installed system path, see #8 . Use forward slash rather than backslash since that has better cross-OS compatibility.
• remove skip_if_not_fileio_available() (that function allowed hyperSpec distribution without test files, but is not needed any more, so should also not be imported by any of the new pacakges)
• make sure tests work
• is test coverage OK?

• file import example read_spe()
• demonstrate spe_showcalpoints()
• what needs to be moved over from hyperSpec?

read_spe() differs from all other file import functions in that it offers two more options for post-processing:

• acc2avg which triggers averaging accumulations (instead of the summation done by the Winspec software)
  i.e. dividing the spectral intensities by the number of accumulations, and
• cts_sec which will divide the spectral intensities by the exposure time

Do we want to keep this functionality or do we prefer that any such non-standard treatment should be done by the user afterwards?

... into inst/extdata/ directory

I think we should change the default xaxis value from "file" to NULL (or "default") as "file" is not a unit of measurement. What do you think @cbeleites?

The other issue is that value "file" is not documented well enough.

• Change the vale and code in all functions.
• Improve documentation by describing what NULL (of "file") really means.

There are many (small) files in inst/extdata directory, that are not used. As they are small, they may be left in the package. What do you think @cbeleites, @sangttruong, @bryanhanson? Are there any plans to use them in e.g., vignettes, unit tests, or examples?

The PDF manual is build-ignored.

I installed hyperSpec 0.100.0 which is going to be released on CRAN and roxut 0.3.23 (current cran version). R CMD check results in this error:

I think, the issue is missing a comment symbol on line 2 in unit test file:

I can confirm that this issue is not present with previous roxut CRAN relaease (v0.2.40)

We should avoid large unnecessary files as only 5 MB are allowed on CRAN: