Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

question about fldgen function conversions from R to python component #60

Open
abigailsnyder opened this issue Jun 11, 2020 · 2 comments

Comments

@abigailsnyder
Copy link

@crvernon @FeralFlows I don't know if this is an oversight, or something that got taken care of in a different part of the code calling the fldgen R functions from python but I noticed in line 908 of components.py, in the run_fldgen function, there is this line

fullgrids = fldgen.generate_TP_fullgrids(emu, resids, tgav)

If I was doing that in R, the final reconstructed full grids would be given in units of Kelvin (the temperature units of the training data) and LogPrecipitation still.

In R, if I wanted units of precipitation, the call I would do is

fullgrids <- fldgen::generate.TP.fullgrids(emu, resids, tgav,
                                  tvarunconvert_fcn = NULL, pvarunconvert_fcn = exp)

I don't know enough of how we go from fldgen::generate.TP.fullgrids in R to fldgen.generate_TP_fullgrids in components.py to know if that happened somewhere else in that process, or if it's not happening at all in the code.

@abigailsnyder
Copy link
Author

Note that there was this issue opened in the fldgen repo (JGCRI/fldgen#32) that sounds kind of related. I closed it out there, because the function does properly convert the mean field back to native units (that is, from logP to P) if you give the function the right arguments to do so. I didn't think about where the use of fldgen might have brought that issue up, so I didn't look outside of the fldgen repo. I checked the order of operations a bunch of times because it is a little finicky, and I recorded my notes in the issue when I closed it out. Always possible I made a mistake though.

But now, I wonder if it came up from looking at these pipeline results and thinking it was an issue with the function itself rather than the call to the function within the pipeline, and so got put in the fldgen repo instead of here. I could also be completely wrong.

@abigailsnyder
Copy link
Author

*I did just remember that tvarunconvert_fcn = NULL, pvarunconvert_fcn = exp) are the default arguments to fldgen::generate.TP.fullgrid, so in R, you wouldn't have to include those arguments. So maybe it is the same in python code calling R functions, sorry, I just don't know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant