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

Add stub ex-scripts, recentering class, and ctest for recentering task #916

Closed
wants to merge 8 commits into from

Conversation

AndrewEichmann-NOAA
Copy link
Collaborator

Dependent on NOAA-EMC/global-workflow#2299 being merged, partially addresses #912

ShastriPaturi
ShastriPaturi previously approved these changes Feb 13, 2024
apchoiCMD
apchoiCMD previously approved these changes Feb 13, 2024
@AndrewEichmann-NOAA AndrewEichmann-NOAA changed the title Add stub ex-script and ctest for recentering task Add stub ex-scripts, recentering class, and ctest for recentering task Feb 15, 2024
Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

Please see comments.

ush/soca/marine_recenter.py Outdated Show resolved Hide resolved
import os
from logging import getLogger
from typing import Dict, List, Any
from wxflow import logit
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from wxflow import logit
from wxflow import Task, logit

import os

from wxflow import Logger, cast_strdict_as_dtypedict
from soca.marine_recenter import MarineRecenter
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment with a TODO: to move marine_recenter.py to ush/python/pygfs/task in the global-workflow so that when the time comes, it is easily achieved.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it helps, it would be:

from pygfs.task.marine_recenter import MarineRecenter


class MarineRecenter():

def __init__(self, config):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add stub documentation and follow typing hints. E.g.

Suggested change
def __init__(self, config):
def __init__(self, config: Dict) -> None:
"""Stub constructor for ocean recentering task
Parameters:
------------
config: Dict
configuration of XYZ
Returns
--------
None

Please see examples of tasks in ush/python/pygfs/task of the global-workflow.

self.task_config = AttrDict(**self.config, **self.runtime_config, **local_dict)

#_soca_ensb_yaml_temp
print(self.task_config)
Copy link
Contributor

Choose a reason for hiding this comment

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

use logger.info or logger.debug
Also, since printing a dictionary, please use pformat

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is temporary for debugging

Comment on lines 39 to 43
FileHandler({'mkdir': [self.task_config.diags,
self.task_config.obs_in,
self.task_config.bkg_dir,
self.task_config.anl_out,
self.task_config.static_ens]}).sync()
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be in a yaml, not in python.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I'm not sure what any of these have to do with the recentering @AndrewEichmann-NOAA . You don't need obs, diags, anl_out or static ensemble ... Let's talk off line, we'll go through the steps that needs to be implemented for this job.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

@AndrewEichmann-NOAA
Copy link
Collaborator Author

Since this is going beyond stub status, I'm closing this PR and holding off until a nominally working version is ready

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.

5 participants