WITec matlab wavelengths

[GregoryHuff]

I was playing around with hySpc.read.mat for the first time and noticed that wavelengths weren't being read into the hyperspec object. Took me a while to figure out I could do this with one line of code...

[bryanhanson]

@GregoryHuff thanks for this report and PR. There's still a lot of loose ends on many of the r-hyperspec... repos, so your input is helpful! Someone will look at this in the near future (there are 4 of us trying to wrap things up).

[GregoryHuff]

OK great. I updated this again.
Not sure you all are aware that there are at least two different formats of WITec matlab files. I put a brief explanation in the comments. More info is here and in WITec help.
The files in inst\extdata\mat.Witec all appear to be DSO type. It would be good to add some non-DSO ones to unit test, I suppose.

[GegznaV]

@GregoryHuff, thanks for contributing.

Not sure you all are aware that there are at least two different formats of WITec matlab files.

The files in inst\extdata\mat.Witec all appear to be DSO type. It would be good to add some non-DSO ones to unit test, I suppose.

I personally do not work with WITec equipment. Could you, please, provide a small file with the format you are talking about? Do that by opening a new issue and dropping a zipped file there (GitHub accepts only a few files extensions, .zip is one of those).

UPDATE: I've opened issue #9 myself. Please, drop the file there.

I put a brief explanation in the comments. More info is here and in WITec help.

This will be helpful.

My remarks on PR:

1. I've updated the code style. Please, use the Tidyverse style in the future. (You can read more on the Tidyverse style here: https://style.tidyverse.org/). If you use RStudio, the easiest way is to fix the current style into the Tidyverse syle is to use the styler addin.
2. Unit tests fail, so something is wrong. @GregoryHuff, did you run R CMD check and unit tests locally, and did they pass?

[GegznaV]

Unit tests must not fail and R CMD check must pass.

[GregoryHuff]

hmm... Unless I am completely off base, there is an issue with the unit tests. This is because (until my update) the wavelengths were not being extracted from the matlab files. You can explore the time-series.mat file yourself to see that wavelength[379] is in fact 686.472415800377, not 379.

[GegznaV]

Could you, please, update unit tests, then so that R CMD check could pass?

[GegznaV]

@GregoryHuff, does the bullet I added to the NEWS.md file that summarizes the essence of this PR correctly?

[GegznaV]

And, please, avoid reverting any changes I made as this let us see the changes of this PR only and not of updates in the develop branch.

[GegznaV]

@cbeleites, do you think this PR is ready to be merged?

[GregoryHuff]

And, please, avoid reverting any changes I made as this let us see the changes of this PR only and not of updates in the develop branch.

Oops. Thought I was reverting a reversion.

[GregoryHuff]

@GregoryHuff, does the bullet I added to the NEWS.md file that summarizes the essence of this PR correctly?

looks good to me.

[cbeleites]

@GregoryHuff do I understand correctly that the DSO format isn't actually Witec's file format but Witec using the Eigenvector DSO format?

In that case, I suggest we add a separate read_mat_DSO() for the Eigenvector file format. It will be easier to tackle this in a separate issue and pull request, though after this one is merged.

[GregoryHuff]

@GregoryHuff do I understand correctly that the DSO format isn't actually Witec's file format but Witec using the Eigenvector DSO format?

In that case, I suggest we add a separate read_mat_DSO() for the Eigenvector file format. It will be easier to tackle this in a separate issue and pull request, though after this one is merged.

Yes, I believe that is the case. You idea makes sense to me, although I don't know of any other companies using DSO.

[GegznaV]

The files in inst\extdata\mat.Witec all appear to be DSO type. It would be good to add some non-DSO ones to the unit test, I suppose.

Do I understand correctly that the current function (read_mat_WITec()) reads the DSO format and have unit tests for that?

So we want to rename it to, say, read_mat_dso(), and create a new function read_mat_WITec() for non-DSO format. I'm a bit confused here (as I did not have time to delve into the peculiarities of the two file formats).

[GegznaV]

I don't know of any other companies using DSO

Could DSO and non-DSO files be identified automatically?
Wouldn't it be better to have an argument, e.g., is_dso with values TRUE, FALSE, and NULL/`"auto" for auto detection?

[GregoryHuff]

I don't know of any other companies using DSO

Could DSO and non-DSO files be identified automatically?
Wouldn't it be better to have an argument, e.g., is_dso wit values TRUE, FALSE, and NULL/`"auto" for auto detection?

In order to differentiate between DSO and non-DSO WITec files, I check for the existence of a field called 'datasetversion'. That's good enough for the case when you know you have a WITec file, but if you want to be able to throw any matlab file in and find out whether it's DSO or not, this probably isn't thorough enough.

[GegznaV]

@cbeleites Do you think, this PR is ready to be merged? Unit tests pass.