JGCRI / hector

The Hector Simple Climate Model
http://jgcri.github.io/hector/
GNU General Public License v3.0
107 stars 45 forks source link

Expose variables & add the lookup function capability #656

Closed kdorheim closed 1 year ago

kdorheim commented 1 year ago

Documentation changes & added a new capability, this PR does not change Hector output

This PR is somewhat miscellaneous but

kdorheim commented 1 year ago

@leeyap it would be great to get your input on the getfxn function, is this helpful? Or would it be better for the function to return the actual function instead of the string function name?

@bpbond your input on getfxn would be much appreciated! Something I am not sure about is the naming of the function and the associated .R scripts, I settled on getfxn to be consistent with getunits but also think that fxn.R script is kinda vague so I am open to other naming suggestions 🤷‍♀ī¸. In data-raw there is a TODO because there is some hard coding that I don't love, here are a few potential courses of action

  1. leave it as it and punt to a later issue
  2. remove the units column from the table
  3. use a try catch to handle the helper functions and parameters that don't have units or dates
  4. replace the data-raw/save-input-params.R with code so that the input table stays up to date with the package which would help with this issue
bpbond commented 1 year ago

Something I am not sure about is the naming of the function and the associated .R scripts, I settled on getfxn to be consistent with getunits but also think that fxn.R script is kinda vague so I am open to other naming suggestions 🤷‍♀ī¸.

Hmm, I don't have an immediate better suggestion. It works.

In data-raw there is a TODO because there is some hard coding that I don't love, here are a few potential courses of action

I see the TODO and responded to it. How does that relate to your courses-of-action list? They seem to be concerned with units/dates (I'm a little unclear on what problem exactly).

kdorheim commented 1 year ago

I guess it would be nice to be able to the fxns table to generate the information in the output table see the vignette/articles/output.hml but that would be a bit more work I may open as a future development issue. Unless you and/or @leeyap have other concerns, then I think this PR can be merged

bpbond commented 1 year ago

That would be my vote. A table with all the names and strings is hugely helpful, even if then the users need to go check about the units, etc.

getfxn does need to be documented somewhere. Is a logical place in a vignette we could add it?

leeyap commented 1 year ago

@kdorheim this is super helpful! I think it makes sense to return the function as a string, what do you mean by "return the actual function?"

bpbond commented 1 year ago

Added @leeyap aș a formal reviewer, as I agree it'd be very useful to get her feedback.

leeyap commented 1 year ago

I agree that the table is very very useful, even without units since that's easy enough to check in R. Otherwise, no comments from me, thanks @kdorheim!

kdorheim commented 1 year ago

We could add a vignette, as of now though it is documented with rxoygen so help(getfxn) will generate documentation and it will automatically be added to the online pkgdown documentation. I've added an example code to that documentation. Do you still think we need a vignette?

bpbond commented 1 year ago

Not for now anyway, no. Good to go from my pov.