lanl / NEONiso

R package for calibrating NEON atmospheric isotope data
https://lanl.github.io/NEONiso/
GNU General Public License v3.0
2 stars 2 forks source link

Iso cor #88

Closed NDurden closed 1 year ago

NDurden commented 1 year ago

Modifying and using calibrate_carbon() to calibrate NEON isotope data (see flow.isoCo2.cor.R for the workflow).

rfiorella commented 1 year ago

A few issues to resolve:

rfiorella commented 1 year ago

@NDurden I've gone through the changes to the package functions, and I think I do see the need now for the changes made to calibrate_carbon.R (stackEddy will take a folder of several files, or a single hdf5 file, but I don't think it works on a list of files, at least based on the documentation).

I think the right place for this code though is ingest_data function and not calibrate_carbon.R, to keep calibrate_carbon.R as short as possible.

The other possibility is that this change shouldn't be in NEONiso at all, but that neonUtilities::stackEddy could be altered in order to handle a list of hdf5 files. @cklunch do you think there would be other users that might need to load, for example, a year of eddy data that isn't in its own folder or in a zip file?

cklunch commented 1 year ago

@rfiorella stackEddy() does work on a vector of filepaths that each point to an H5 file. I haven't really advertised that option because you miss out on some of the metadata if you do it that way - e.g. the updates in v2.3.0 that give you the science review flags and the citation info won't work, because those metadata are sourced from other files in the download package. Although of course that is also true if you stack from a single H5 file, I guess in that case it seems like less of a big deal since people presumably know they're working with minimal data.

Try it out, let me know if it works for your use case!

rfiorella commented 1 year ago

@cklunch Sure thing, I'll write a couple tests and see if it works as expected for this case. Thanks!

@NDurden I'll be merging this PR if you're satisfied with the changes I've made after adding these tests and moving the code to ingest data. Let me know if you have any concerns!

NDurden commented 1 year ago

@rfiorella sounds good to me and let me know if any changes that I need to make. Thanks!

NDurden commented 1 year ago

@rfiorella thanks for working on these. I will test my workflow and will let you know how that goes.

NDurden commented 1 year ago

@rfiorella I tested my workflow with latest merge and it gave me the error messages 'separate_wider_delim' is not an exported object from 'namespace:tidyr'. The error seems to occur in stackEddy. I tested on neonUtilities (version 2.3.0). Do you have any idea what happened?

rfiorella commented 1 year ago

@NDurden hmm, I have not seen that error message before. What version of tidyr is installed on the machine you're working on? I think these functions are quite new in tidyr, so you might try updating tidyr if it is < 1.3.0

@cklunch any other ideas?

NDurden commented 1 year ago

@rfiorella @cklunch that error definitely came from the old version of tidyr. However, it gave me another error message which came from the site name (https://github.com/NEONScience/NEON-utilities/blob/f1473397404e1a596d6b69f9d8c16ee740b8372a/neonUtilities/R/stackEddy.R#L450) I will try to work around by adjusting my workflow. I will keep you posted.

cklunch commented 1 year ago

@NDurden Make sure all the packages neonUtilities is dependent on are up to date, if they're not it can definitely cause problems. If you still run into issues let me know. Thanks!