pierreroudier / opusreader

Reading OPUS binary files in R
GNU General Public License v3.0
4 stars 5 forks source link

Refactoring of the `extract` option #2

Closed pierreroudier closed 3 years ago

pierreroudier commented 3 years ago

@philipp-baumann Possibly a controversial refactoring: I'd like to refactor the way options are passed to extract, so that they are more explicit/user-friendly, and avoid mixing lower and upper case.

My suggestions:

For backwards compatibility, we could still support spc, ScSm, ScRf, IgSm, and IgRf, but I'd think using more explicit options will make the package easier to use.

Thoughts?

pierreroudier commented 3 years ago

Another thing I'm thinking to do is to rename the option from extract to type.

pierreroudier commented 3 years ago

Additional question: at the moment there is a mismatch between the type of spectra extracted (spc, ScSm, ScRf, IgSm, IgRf) and the name of the slot in the list returned by opus_read that contains that spectra (which is either spc, sc_sm, sc_rf, ig_sm, ig_rf).

Shouldn't they be the same (so the returned list would have slots names spc, ScSm, ScRf, IgSm, IgRf)?

philipp-baumann commented 3 years ago

Additional question: at the moment there is a mismatch between the type of spectra extracted (spc, ScSm, ScRf, IgSm, IgRf) and the name of the slot in the list returned by opus_read that contains that spectra (which is either spc, sc_sm, sc_rf, ig_sm, ig_rf).

Shouldn't they be the same (so the returned list would have slots names spc, ScSm, ScRf, IgSm, IgRf)?

The names of the spectra types returned as elements of the list should indeed be the same. The CamelCase names correspond to the names of the spectra blocks that are displayed in the Bruker OPUS software. We could either allow both classical and new represenations of names in the output, or convert to snake_case as in the code style used no matter whether old or new names are used in opus_read(). What do you think? In any case, I think it would be good that we further document or keep the reference to the names in the Bruker OPUS software.

philipp-baumann commented 3 years ago

My suggestions:

* `spc` (default): `absorbance`

The final spectrum (before or after atmospheric correction of CO2 and water lines) can be absorbance or reflectance, depending on what is in the configuration file .xpm or what the user is actively setting before doing measurements. spc_nocomp is for spectra that were not background corrected. This occurs if background correction was done, so that means one can catch both in that case. This seems important depending on how a library and a platform deals with such things. I think would have to see whether we need further tweaks to update the metadata with the y-type of the spectra.

pierreroudier commented 3 years ago

The names of the spectra types returned as elements of the list should indeed be the same. The CamelCase names correspond to the names of the spectra blocks that are displayed in the Bruker OPUS software. We could either allow both classical and new represenations of names in the output, or convert to snake_case as in the code style used no matter whether old or new names are used in opus_read(). What do you think?

Consistency is the key! If we expect users to be familiar with CamelCase names, we may as well stick with them (even though I'm not a great CamelCase fan myself!). My main worry is consistency:

Note that if we were to go for sc_sm, sc_rf, we could still make the CamelCase options work for backwards compatibility.

In any case, I think it would be good that we further document or keep the reference to the names in the Bruker OPUS software.

I agree, good documentation is probably the main answer to this question. Incredibly little content on the web!

The final spectrum (before or after atmospheric correction of CO2 and water lines) can be absorbance or reflectance, depending on what is in the configuration file .xpm or what the user is actively setting before doing measurements.

Ok. Again, probably best to document this thoroughly. Could maybe add a @details slot in the docs?

spc_nocomp is for spectra that were not background corrected. This occurs if background correction was done, so that means one can catch both in that case. This seems important depending on how a library and a platform deals with such things. I think would have to see whether we need further tweaks to update the metadata with the y-type of the spectra.

Thanks, will update the docs accordingly.

philipp-baumann commented 3 years ago

I will address renaming of extract argument to type or spectrum_type in a separate commit.

pierreroudier commented 3 years ago

@philipp-baumann Thanks for PR https://github.com/pierreroudier/opusreader/pull/7, I just merged it :)

pierreroudier commented 3 years ago

@philipp-baumann

Going forward with the re-factoring (our last item before initial CRAN submission!), here is my proposal:

Thoughts?

pierreroudier commented 3 years ago

Implemented change from extract to type in https://github.com/pierreroudier/opusreader/commit/e5784a8f2f3fc35e984ff65a206801b2c23d2678

pierreroudier commented 3 years ago

Update on the possible options for type (proposition):

with Bruker options (ScSm, ScRf, IgSm, IgRf) "silently supported"

pierreroudier commented 3 years ago

Implemented change in type naming (as above) in https://github.com/pierreroudier/opusreader/commit/01dfd5a496a76e149ac9a7301720a7047764031b

pierreroudier commented 3 years ago

@philipp-baumann The last commit is resolving the issue, but happy to re-open it if you are unhappy with aspects of the new naming.