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

Consolidate cmor setup 273 #275

Merged
merged 2 commits into from
Sep 4, 2024
Merged

Consolidate cmor setup 273 #275

merged 2 commits into from
Sep 4, 2024

Conversation

TonyB9000
Copy link
Contributor

@TonyB9000 TonyB9000 commented Aug 30, 2024

CMOR SETUP CONSOLIDATION:

(Addresses issue #273)
(Closes issue #273)

The motivation for cmor.setup reconfigurations is that this cmor function was called from 7 separate locations:

the setup_cmor() in util.py
the setup_cmor() in mpas.py
the _setup_cmor_module() in handler.py
and 4 raw calls to cmor.setup in the areacella.py, clisccp.py, orog.py and sftlf.py variable handlers.

Aside from unnecessary replication of codes, each call to cmor.setup invokes a cmor-internal "logger" setup, whose only external accessibility is the name of the logfile to supply. There is no ability to alter its propagate(True/False), date-format or other attributes. (I believe it is responsible for the "duplicated log messages" problem, as only those messages with the CMOR date-format appear to be duplicated.)
Describe the solution you'd like

Now, there are only TWO functions that invoke cmor.setup: util.setup_cmor(), and handler._setup_cmor_module(). The latter is applied to every handler managed through the "handlers.yaml" construct. The former handles all other variables. The function in mpas.py has been eliminated.
Describe alternatives you've considered

No response
Additional context

No response

@TonyB9000
Copy link
Contributor Author

@tomvothecoder This URL is generated by the console command "git push --set-upstream origin consolidate_cmor_setup_273" and don;t know how to link it to the "issue 273" you created, except to mention it at the top.

I've conducted both non-MAP and MPAS CMIP6 variable generation successfully.

QUESTION 1: Should I wait until this issue is resolved BEFORE working on Redesign Logging (so changes can be applied to the rebased e2c), or just go ahead an apply logging work to the original main and merge thereafter?

QUESTION 2: I want to issue some "cleanup" changes on a separate branch. Would that constitute a Bug or a Feature, or something else, issue-wise?

@tomvothecoder
Copy link
Collaborator

Thank you Tony for this PR. I will review this PR in the next few days.

and don;t know how to link it to the "issue 273" you created, except to mention it at the top.

Update/add "closes #273" to the description

QUESTION 1: Should I wait until this issue is resolved BEFORE working on Redesign Logging (so changes can be applied to the rebased e2c), or just go ahead an apply logging work to the original main and merge thereafter?

You can start the logger PR before this is merged if you are comfortable rebasing. Otherwise wait until this PR is merged to not deal with rebasing. The changes in this PR seem minimally invasive, just repeated across many files.

QUESTION 2: I want to issue some "cleanup" changes on a separate branch. Would that constitute a Bug or a Feature, or something else, issue-wise?

It would be considered a "Refactor", which there is no template for. You can open a Feature issue and replace "feature" with "refactor".

@TonyB9000
Copy link
Contributor Author

@tomvothecoder Thanks Tom! Refactor is a good term - should be an option.

Copy link
Collaborator

@tomvothecoder tomvothecoder left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @TonyB9000.

@TonyB9000
Copy link
Contributor Author

@tomvothecoder Thanks Tom! One down ...

@TonyB9000 TonyB9000 merged commit d105787 into master Sep 4, 2024
5 checks passed
@TonyB9000 TonyB9000 deleted the consolidate_cmor_setup_273 branch September 4, 2024 22:08
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.

2 participants