Conversation
rplzzz
left a comment
rplzzz
left a comment
There was a problem hiding this comment.
Looks good so far. Since we're still seeing a discrepancy in the forcing totals, I've tagged it as WIP until we track down the source. If it's another minor fix, we can roll it in with this PR.
…agents equals the total climate RF.
The halocarbon forcings are supplied by the halocarbon components, not the forcing component, and the former do not know how to correct their values to be relative to the base year. To rectify this, we've added a new set of capabilities for adjusted halocarbon forcings, which are reported by the forcing component and given relative to the base year. The R interface has been changed to report the adjusted forcings whenever halocarbon forcings are asked for. There is no provision for getting the unadjusted halocarbon forcings in R because Hector does not provide unadjusted forcings for other GHGs, and so there isn't really anything you can do with the unadjusted halocarbon values.
|
@kdorheim I can't add you as a reviewer, since this is originally your PR, but if you would, please, take a look at my changes (you might want to filter to just the commit that I added), and let me know if you see anything that could use some improvement. |
|
The code won't build with GCC because I used a C++-11 feature, and g++ is configured to do C++-0x compatibility. That should be an easy fix to the makefile. I'm not sure why the R build succeeds, whether it's because w'ere using clang on the travis R build, or whether the default flags for g++ are different under Rcpp. Either way, while we're at it, we should probably configure the Rcpp build explicitly to specify C++-11. We've ben getting compiler warnings about C++-11 features the whole time, and we wouldn't want someone's build to fail because their compiler didn't get the memo. I probably won't get to any of this until after my telecon tomorrow afternoon. In the meantime, this branch should be usable, just not quite ready to merge. Addendum: It looks like the R build on Appveyor is also failing because of this, so we definitely need to update the standard flags in the Rcpp build. |
I think the recommended way to do this is to add the following line to the
I think this is just a matter of adding |
|
Well, on the windows side we still get warnings about C++-11 features, but at least it builds. I guess any compile you can walk away from is a good one. On the mac R builds we're now building cleanly, with no warnings at all, so I'm calling this a win. |
rplzzz
left a comment
rplzzz
left a comment
There was a problem hiding this comment.
I think everything appears to be in order; however, since I added some of the changes here, it really should have another pair of eyes on it before we merge.
bpbond
left a comment
bpbond
left a comment
There was a problem hiding this comment.
Looks fine generally but worth considering nomenclature.
Replace "adjusted" with "relative", where feasible.
4 participants
Change the bindings between R and C++ code so that the RF_H2O() refer to H2O RF.