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

Three patches for time series generation #218

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

mnlevy1981
Copy link
Collaborator

  1. Added case argument to env_workflow.get_job_specs()
  2. explicitly ignored last two arguments returned from
    env_mach_specific.get_mpirun() -- shouldn't be necessary, but I was seeing
    ValueError: too many values to unpack and the error went away with the
    additional _s
  3. Remove indexing from options.debug in tseries_generator, since
    parser.add_argument() explicitly casts it as int; avoids
    TypeError: 'int' object has no attribute '__getitem__'

1. Added "case" argument to env_workflow.get_job_specs()
2. explicitly ignored last two arguments returned from
   env_mach_specific.get_mpirun() -- shouldn't be necessary, but I was seeing
   "ValueError: too many values to unpack" and the error went away with the
   additional _s
3. Remove indexing from options.debug in tseries_generator, since
   parser.add_argument() explicitly casts it as int; avoids
   "TypeError: 'int' object has no attribute '__getitem__'"
@@ -37,7 +37,7 @@ def _main_func(description):
"queue" : case.get_value("JOB_QUEUE", subgroup=job),
"unit_testing" : False
}
executable, mpi_arg_list = env_mach_specific.get_mpirun(case, mpi_attribs, job, overrides=overrides)
executable, mpi_arg_list, _, _ = env_mach_specific.get_mpirun(case, mpi_attribs, job, overrides=overrides)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't really understand why this change was necessary. Maybe something weird from python 2.7?

@mnlevy1981
Copy link
Collaborator Author

I needed to make these changes to run a case from cesm2_2_beta05, but I suspect it will break compatibility with cesm2.1... which probably isn't ideal if CMIP runs are making use of --workflow timeseries. I've got a sandbox that works for me, so I'm in no rush to see these changes back on master

@mnlevy1981
Copy link
Collaborator Author

Fixes #217 (I don't have permission to link the issue to this PR)

@bertinia
Copy link
Contributor

@mnlevy1981 - I'm going to wait on merging this PR as it's not clear what direction the
postprocessing is going post CMIP6 and I don't want to break the current working master.

@lvankampenhout
Copy link

Regarding the indexing of the debug parameter, I agree that this should be removed. However, the proposed solution will lead to DEBUG statements when '0' is specified, since the result is stored in a list [0] and this list renders to True in a statement like: if (debug): .

I propose this alternative:

-    parser.add_argument('--debug', nargs=1, required=False, type=int, default=0,
+    parser.add_argument('--debug', required=False, type=int, nargs='?', default=0, const=2,

this will work as expected in all cases, and even works when --debug is specified but no value is given (it will take the value of 2). See nargs documentation.

lvankampenhout added a commit to lvankampenhout/CESM_postprocessing that referenced this pull request Sep 30, 2022
@dabail10
Copy link
Collaborator

dabail10 commented Oct 5, 2023

@mnlevy1981 I know this is old, but could you resolve the conflicts here?

@dabail10 dabail10 merged commit 16211db into NCAR:master Dec 8, 2023
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

Successfully merging this pull request may close these issues.

4 participants