search

dklinges9 / mcera5

mcera5
12 stars 9 forks source link

added error trapping to .nc_to_df to check space-time representation #2

Closed dklinges9 closed 3 years ago

dklinges9 commented 4 years ago

Hi James,

I've been using mcera5 for the global soil temp mapping that Ilya has been involved with, and it's been of great use! I did however scratch my head for a few hours last week trying to figure out what was failing with a few queries I ran, only at long last to discover I had simply asked for time points not represented in the netCDF. So here I've added a few checks to .nc_to_df that will throw an error to the user if they want something the netCDF can't provide....

of course feel free to change the error messages, move the code up to another function, or such. If you don't mind, I may submit further pull requests for other modifications I've been thinking about– if it would be of help!

Cheers, Dave Klinges

PS: might be worth giving this a quick try on your end just to make sure I didn't screw up the error checking. I use nc_open() from ncdf4, which I'm pretty sure is a dependency of tidync() so would need to be loaded by the user anyways.

everydayduffy commented 3 years ago

Hi Dave,

Thanks for putting the time into making this a formal pull request. I will get round to having a look at some point, but just a little snowed under with other things at the moment. Will get back to you at some point in the near future! Cheers, James.

everydayduffy commented 3 years ago

Hi @dklinges9 ,

I've spent today working on the package and have overhauled some of the function code. To that end, the worker functions are now stored in internal.R. I wondered if you might like to add your error trapping code to nc_to_df & nc_to_df_precip as a pull request? That would be really handy! I have added ncdf4 as an import for the package too. No worries if you don't have time, just let me know either way.

dklinges9 commented 3 years ago

Hi @everydayduffy ,

So sorry that I missed this– I was certainly awaiting your response yet expected I'd get an email notification, I'll have to fix that.

This was a pull request, albeit to functions.R instead of internal.R– should I submit a new one then? Hopefully this would still be relevant and helpful.

everydayduffy commented 3 years ago

Hi @dklinges9 , happy new year!

Yeah sorry, about changing the structure of the package, I'm happier with it now! If you have time to rework the pull request, that would be cool, no worries if it's too time consuming though.

dklinges9 commented 3 years ago

Sure, will submit a new one now.