Addressing assorted issues on the way to v3

kdorheim commented 1 year ago

This PR is a bit of a Hodge podge addressing remaining v3 issues

633 add publication featuring Hector [documentation change]
563 add citation to leeya's cmip6 data [documentation change]
340 & #539 update default hector parameters [changes output behavior]
495 add citation file hat tip to @crvernon

Due to the changes in the default parameters, we will see changes in Hector output behavior. I after we get Leeya bot results I will update the comp data just wanted to include a figure with the comparison with observations.

kdorheim commented 1 year ago

@leeyap and @bpbond any idea why the leeybot figures are not being pushed to the PR?

leeyap commented 1 year ago

Hmm, weird. Looking at the check, it seems the graphs were posted here? https://github.com/JGCRI/hector/commit/19f09b37e133ff1fa5c140fea20104debf9f6193#commitcomment-89269587

Not sure why they aren't showing up on the PR, let me take a closer look...

leeyap commented 1 year ago

Did the cml package update? What happens in https://github.com/JGCRI/hector/pull/658/commits/87f206f74a68af81d1f3923012161a11d0ec9b13?

kdorheim commented 1 year ago

yea I got error messages stating that the previous commands had been deprecated and update to the commands that are now being used 🤷‍♀️ but may be that wasn't the correct thing to do

leeyap commented 1 year ago

Hmm I see, yes, that should be right!

Maybe try installing the specific version of cml?

leeyap commented 1 year ago

@kdorheim Idea: I wonder if they changed the way you specify push, PR, etc. I glanced at the documentation and the example they show has this: Which is different from how we have ours set up

github-actions[bot] commented 1 year ago

b813706

Differences in Hector outputs

Hello, this is leeyabot! 🤖

The current pull request's outputs differ from 3.0.0 (2a0fd4ff) as follows:

	R squared	NRMSE
CO2_concentration	0.996	0.024
global_tas	0.996	6.408
RF_CO2	0.996	4.426
RF_tot	0.996	3.904

kdorheim commented 1 year ago

Alright so for future selves and those following along there had been an update to cml which was causing the issues with leeya bot.

kdorheim commented 1 year ago

@bpbond leeyabot is showing the differences in output as we expected updating the default parameters will cause a large change in output behavior. If it looks good to you, I will go ahead and update the comparison data so that we can pass tests and merge into v3.

kdorheim commented 1 year ago

@bpbond "Command Line Hector / ubuntu (pull_request)" should we merge into dev v3 branch or address this failing test first?

bpbond commented 1 year ago

Weird, I wonder what caused that. Up to you, but I'd vote fix here rather than start erroring v3_dev. I can take a look at it if useful—lmk.

kdorheim commented 1 year ago

I don't think I have time to dig into this, this week if you could address it that would be a huge help!

bpbond commented 1 year ago

@kdorheim You made this change:

But when the test_hector.sh script tries to turn on a CO2 constraint (in line 57 of that script), it's looking for

sed 's/;[[:space:]]*CO2_constrain=csv:tables\/ssp245_emiss-constraints_rf.csv/

Its search string no longer matches line 63 in the INI file, so it can't find the correct line, and by design dies in line 58.

bpbond commented 1 year ago

Also it looks like the data files for tas_constrain, RF_tot_constrain, and NBP_constrain were removed, but their columns never made it into the various "emiss-constraints_rf.csv" files. So in those cases, the script can't find the data and dies. I am pushing the script naming fix but will let you handle this second issue!

bpbond commented 1 year ago

@kdorheim I realized maybe this ☝️ may be unclear or grumpy-sounding (which I didn't intend!). Summarizing:

The sed commands in the shell script are looking for a particular file pattern, which has now changed. I have updated them in d4ad069b
But, some of them look for individual constraint test files: "tas_historical.csv" (line 80) and "nbp_output.csv" (line 94) for example. These no longer exist—they were removed in 37802731). They're explicitly test files so it doesn't seem right to incorporate them into the "rcmip_ssp245_emiss-constraints_rf.csv" file...anyway, I wasn't sure how you wanted to organize this, so didn't fix.

kdorheim commented 1 year ago

Sigh... yes deleting the constraint files was a mistake I will add them back into the appropriate directories. As for the file names ugh annoying I am trying to sort through some potential input problems & then I think it would be helpful to have a quick chat to about naming conventions & default Hector scenarios 😬

bpbond commented 1 year ago

Re quick chat, before our Wednesday meeting? If so my calendar is up to date. Thanks

kdorheim commented 1 year ago

@bpbond i don't understand the bash ./test_hector.sh ./src/hector is passing on my local machine :(

bpbond commented 1 year ago

@kdorheim It's actually not passing—but it's unfortunately easy to miss. On GA:

Nice and clear! On my local machine:

At first glance, no visible error message...BUT normally this script ends with an "All done" message. That doesn't appear, and at second glance, it started to test the RF constraint parameter, but then died via the exit 1 in line 88, because it tried to sed a change but couldn't (i.e., it couldn't find the line).

THIS ISN'T OBVIOUS AT ALL. 🤦 Sorry! I have pushed 5f69d5ba (edit: and also 1450024c lol) so now this happens:

...hopefully making it nice and obvious that an error occurred, and why.

JGCRI / hector

Addressing assorted issues on the way to v3 #658

633 add publication featuring Hector [documentation change]

563 add citation to leeya's cmip6 data [documentation change]

340 & #539 update default hector parameters [changes output behavior]

495 add citation file hat tip to @crvernon

Differences in Hector outputs