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

ILAMB task revision #230

Merged
merged 3 commits into from
Apr 6, 2022
Merged

ILAMB task revision #230

merged 3 commits into from
Apr 6, 2022

Conversation

forsyth2
Copy link
Collaborator

@forsyth2 forsyth2 commented Mar 28, 2022

Add ILAMB task. Revision of #197. Resolves #133. Resolves #134. A future pull request will handle #229.

@forsyth2 forsyth2 added the semver: new feature New feature (will increment minor version) label Mar 28, 2022
@forsyth2 forsyth2 self-assigned this Mar 28, 2022
@forsyth2
Copy link
Collaborator Author

It took some debugging to create the single initial commit for this pull request. I've listed what I believe to be the necessary steps below:

$ git fetch upstream e3sm2cmip_ilamb
$ git checkout -b e3sm2cmip_ilamb upstream/e3sm2cmip_ilamb
$ git rebase --rebase-merges -i upstream/main
# There are 8 commits:
# pick
# change to fixup
# change to fixup
# change to fixup
# merge -C
# pick
# change to fixup
# change to fixup
# Now, there are 3 commits.
# See https://stackoverflow.com/questions/32840872/convert-merge-into-rebase-without-having-to-perform-the-merge-again
$ git reset --soft HEAD~2
$ git log
# Now, only the first commit shows up
$ git commit --no-verify # Effectively turns the merge commit (and the commit after) into a single regular commit
$ git rebase -i upstream/main
# There are 3 commits:
# pick
# change to fixup
# change to fixup
# Now, there is 1 commit.
$ git commit --amend
# change commit title to "ILAMB task initial commit"
$ git push forsyth2 e3sm2cmip_ilamb
# Create this pull request
# Confirmed that this pull request adds 1,580 lines and removes 2 lines, just as #197 does.

@chengzhuzhang chengzhuzhang changed the title ILAMB task ILAMB task rev. Mar 29, 2022
@forsyth2 forsyth2 changed the title ILAMB task rev. ILAMB task revision Mar 29, 2022
@forsyth2 forsyth2 force-pushed the e3sm2cmip_ilamb branch 2 times, most recently from c8f863c to 83d5f5c Compare April 5, 2022 22:48
@forsyth2 forsyth2 marked this pull request as ready for review April 5, 2022 23:10
@forsyth2
Copy link
Collaborator Author

forsyth2 commented Apr 5, 2022

@chengzhuzhang This pull request contains two commits. The first is all the work you did on #197. The second is my revisions: 3210894 -- that's what needs a review. Once we merge this, I'll update the expected files for the integration tests.

Notable changes:

  • Added parameters (and defaults) for ts_atm_subsection and ts_land_subsection. This enables multiple ts subtasks to be run (e.g., with different values for ts_fmt).
  • Changed location of setting export to ALL.
  • Made sure cmip_metadata and ts_fmt were defined in the appropriate places.
  • Added code to move visuals to web server.
  • Changed tmp directory name to avoid name collisions.

@forsyth2 forsyth2 requested a review from chengzhuzhang April 5, 2022 23:17
@forsyth2
Copy link
Collaborator Author

forsyth2 commented Apr 6, 2022

Also note I'll handle #229 in a follow-up pull request.

@@ -33,6 +33,12 @@ years = "1850:1854:2",
input_files = "eam.h0"
input_subdir = "archive/atm/hist"

[[ atm_monthly_180x360_aave_ilamb ]]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be [[ atm_monthly_180x360_aave ]]? I think the logic is if ts_fmt=cmip, both nco convention time series and cmip time series are generated.
And this sub session can be both a dependency of both e3sm_diags and ilamb.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it must have been some issue in debugging that made me think it required a separate subtask. It looks like it works fine using the same subtask.

Copy link
Collaborator

@chengzhuzhang chengzhuzhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Ryan, the PR looks good, I only has one query left as a single line comment. Thanks!

@forsyth2 forsyth2 merged commit fe5d728 into E3SM-Project:main Apr 6, 2022
@forsyth2 forsyth2 deleted the e3sm2cmip_ilamb branch April 6, 2022 22:52
This was referenced Apr 6, 2022
@forsyth2
Copy link
Collaborator Author

forsyth2 commented Apr 7, 2022

Expected files for integration tests (https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_test_complete_run_www/) have been updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: new feature New feature (will increment minor version)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add task for ILAMB Add task for e3sm_to_cmip
2 participants