-
Notifications
You must be signed in to change notification settings - Fork 47
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
Addressing a handful of open github issues #741
Conversation
Differences in Hector outputsHello, this is The current pull request's outputs do not differ from 3.1.1 (d931a00). |
Differences in Hector outputsHello, this is The current pull request's outputs do not differ from 3.1.1 (d931a00). |
Differences in Hector outputsHello, this is The current pull request's outputs do not differ from 3.1.1 (d931a00). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! I don't think we want to track the output files, though.
.gitignore
Outdated
@@ -64,7 +64,7 @@ libs/ | |||
*.suo | |||
|
|||
# Output | |||
output/*.csv | |||
output/tracking*.csv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with this as way to address #734 is that running an unmodified model will produce modified (from git's point of view) files, because of the timestamp at the top of the outputs...which I don't think is the behavior we want. It seems better to create an example_outputs/
directory, perhaps as a subdirectory of output/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm that is a good point, do we want to add that or does it make sense to include the output streams as part of the release materials?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either works, but I like your suggestion.
README.Rmd
Outdated
@@ -58,7 +58,8 @@ hector_tas_results$scenario <- ifelse(hector_tas_results$year <= 2016, "historic | |||
ggplot(hector_tas_results) + | |||
geom_line(aes(year, value, color = scenario), linewidth = 1) + | |||
theme_bw(base_size = 15) + | |||
labs(color = NULL, x = NULL, y = expression("Temperature Anomaly ("~degree~"C)")) + | |||
theme(axis.title = element_text(size=12))+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super picky, but please size = 12
Differences in Hector outputsHello, this is The current pull request's outputs do not differ from 3.1.1 (d931a00). |
Differences in Hector outputsHello, this is The current pull request's outputs do not differ from 3.1.1 (d931a00). |
@rgieseke you are so right! Thanks for catching that! |
Differences in Hector outputsHello, this is The current pull request's outputs do not differ from 3.1.1 (d931a00). |
Commits in the PR are minor with respect to Hector's model behavior. These changes mostly impact documentation and such.
The following github issues are addressed here
Remaining to dos