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

Output switches for output volumes of iHAMOCC #478

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jmaerz
Copy link
Collaborator

@jmaerz jmaerz commented Feb 5, 2025

Hi @JorgSchwinger and @TomasTorsvik,
since we discussed this briefly after lunch, I started to implement at least the required switch to set different iHAMOCC output options. As is, it works for the thus far example variable SRF_PHOSPH while it doesn't change any default behavior. This is just for demo and need to be concluded via introducing the settings throughout namelist_definition_blom.xml (in another PR, ones defined the output settings).

If you would opt for a different switch name, let me know. Also about the different options (i.e. adding a default option).

@jmaerz jmaerz added enhancement New feature or request iHAMOCC Issue mainly concerns the iHAMOCC code base labels Feb 5, 2025
@jmaerz jmaerz added this to the release v1.8.0 tag milestone Feb 5, 2025
@jmaerz jmaerz self-assigned this Feb 5, 2025
@jmaerz jmaerz mentioned this pull request Feb 5, 2025
19 tasks
Copy link
Contributor

@JorgSchwinger JorgSchwinger left a comment

Choose a reason for hiding this comment

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

Thanks for doing this so quickly. Do you think it would be feasible to write a script to roll this out to all variables?

@@ -79,6 +79,7 @@ def buildnml(case, caseroot, compname):
blom_coupling = case.get_value("BLOM_COUPLING")
blom_tracer_modules = case.get_value("BLOM_TRACER_MODULES")
blom_atrc = case.get_value("BLOM_ATRC")
hamocc_output = case.get_value("HAMOCC_OUTPUT")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest a more self-explaining name, e.g. HAMOCC_OUTPUT_VOL(UME) or HAMOCC_OUTPUT_SIZE

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed to HAMOCC_OUTPUT_SIZE

<value>0,2,2</value>
<value hamocc_output='small'>0,0,2</value>
<value hamocc_output='medium'>0,2,2</value>
<value hamocc_output='large'>2,2,2</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

For daily output it needs to be 4. Or more precisely, whenever more than one time-slice is written into one file, one cannot use 2. The compressed format with offset and scale-factor does not work properly in this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you re-check? The newly pushed commit should have the right output, right?

@jmaerz
Copy link
Collaborator Author

jmaerz commented Feb 5, 2025

Hi @JorgSchwinger , I am just about investigating how to best do this via a xml-parse script... - don't now, how deep the rabbit hole I want to go, though. I'll keep you updated.

@jmaerz
Copy link
Collaborator Author

jmaerz commented Feb 5, 2025

Ok, I technically have a parser routine that allows to set those values for all outputs. However, I wonder, whether this is really the way to go. I'll share the python script that I have (requires minor manual modifications afterward - didn't get some white lines correct...).

@jmaerz
Copy link
Collaborator Author

jmaerz commented Feb 7, 2025

Hi @JorgSchwinger and @TomasTorsvik , how to best proceed? - I feel not being the person who should decide about the volumes for the individual output variables (only have some idea about certain ones). I would propose to declare one default flag in addition to small, medium, large, where we enter the current values - so no changes to the current version. This I would merge. Then, if there is time, to go through the variables, setting just the output values manually (i.e. the full structure can be generated via the script, I sent around).

@TomasTorsvik
Copy link
Contributor

Looks good to me. I think a default option with the current values would be useful.

@jmaerz
Copy link
Collaborator Author

jmaerz commented Feb 7, 2025

I would have a routine ready that basically performs such changes for any iHAMOCC variable (left new, right old):
xml-change

Note to myself: Requires default as option in the compsets

@JorgSchwinger
Copy link
Contributor

Hi @JorgSchwinger and @TomasTorsvik , how to best proceed? - I feel not being the person who should decide about the volumes for the individual output variables (only have some idea about certain ones). I would propose to declare one default flag in addition to small, medium, large, where we enter the current values - so no changes to the current version. This I would merge. Then, if there is time, to go through the variables, setting just the output values manually (i.e. the full structure can be generated via the script, I sent around).

Why not define one of small/medium/large as the default? I would not use the current values as default, but rather something with less output. Given that storage is scarce, I would think it's better that the user needs to actively increase the storage volume if this is desired.

I will come up with a suggestion as to what the categories should contain, but I'll have to do this later (next week)

@@ -189,6 +190,7 @@ def buildnml(case, caseroot, compname):
config["blom_coupling"] = blom_coupling
config["blom_tracer_modules"] = blom_tracer_modules
config["blom_atrc"] = blom_atrc
config["hamocc_output_size"] = hamocc_output_size if is_test == False else 'dummy' # choose is_test, if is_test==True
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this is necessary - the parser will have a logic of taking the last match that is found? At least otherwise, the existing is_test and empty_hist logic wouldn't work either.

@jmaerz
Copy link
Collaborator Author

jmaerz commented Feb 7, 2025

Hi @JorgSchwinger , I am happy, when you flee out the desired output volumes - I just see that there are cases (e.g. CO2 fluxes come to my mind) where the current value is quite different from small/medium/large as compared to the other variables. A default would allow to keep the output as is and having standardized other output volumes. But up to you - it's just needed to manually go through, if there are special wishes/needs - it was just a thought to roll this out now and enabling an easy comparison, when manually adjusting the stuff to your/our needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request iHAMOCC Issue mainly concerns the iHAMOCC code base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants