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 method of specifying streams via yaml #50

Merged
merged 6 commits into from
Jun 18, 2021

Conversation

mgrover1
Copy link
Contributor

@mgrover1 mgrover1 commented Jun 9, 2021

Addresses discussion #46 - main changes include

  • adding a stream_defaults.yaml file which includes
    • stream
      • component
      • default_frequency
        For each of the cesm parsers, the user can specify a yaml file following the format (example - pop.h)
pop.h
    component: atm
    default_frequency: month_1

@andersy005
Copy link
Member

andersy005 commented Jun 10, 2021

@mgrover1, this new approach implies that if you want to use/add new streams, the user must redefine all the other streams + add their own streams to the stream_defaults.yaml file that they pass to to the parsing function? In other words, this isn't updating the existing list of streams but overriding the entire list of default streams?

@mgrover1
Copy link
Contributor Author

@andersy005 that is correct - it overrides the list of default streams. This way, the user can make sure it isn't making assumptions it is not supposed to (helpful when working with CESM1 especially)

@andersy005
Copy link
Member

Isn't this laborious? :) I'm imagining a case scenario in which the user just wants to add a missing stream to the list but we are asking them to redefine the existing ones too. Could we come up with a compromise? :) I'm asking because I think the majority of CESM runs are probably using the same stream configurations (I could be wrong) and the case scenario in which one needs to override the entire list is an exception.

@mgrover1
Copy link
Contributor Author

Good point... how would we allow the user to override a single stream then? What would be the alternative then? In order for one to change the stream + frequency settings, they would need to submit an issue here, or change it locally then reinstall the package (which can be challenging)

@mgrover1
Copy link
Contributor Author

This is an example of some notes I used to make sure the DPLE catalog was parsed together correctly:

  • cam.h1
    • atm
    • day_1
  • cam.h2
    • atm
    • hour_6
  • cam.h0
    • atm
    • month_1
  • cice.h1
    • ice
    • day_1
  • cice.h
    • ice
    • month_1
  • clm2.h1
    • lnd
    • day_1
  • clm2.h0
    • lnd
    • month_1
  • pop.h.ecosys.nyear1
    • ocn
    • year_1
  • pop.h.nday5
    • ocn
    • day_5
  • pop.h
    • ocn
    • month_1
      This took ~5 minutes to compile - I think it is reasonable from a user perspective to be responsible for checking to make sure the frequency is correct for each of the streams, especially for CESM1

@mnlevy1981
Copy link

Good point... how would we allow the user to override a single stream then? What would be the alternative then? In order for one to change the stream + frequency settings, they would need to submit an issue here, or change it locally then reinstall the package (which can be challenging)

Can you let the user pass in a subset of streams in a yaml file that you read in as user_streams_dict and then call _STREAMS_DICT.update(user_streams_dict)?

I think that's exactly the functionality we want:

  1. keys in _STREAMS_DICT that also appears in user_streams_dict will have the user_streams_dict values
  2. keys in _STREAMS_DICT that are not in user_streams_dict will remain unmodified
  3. keys in user_streams_dict that are not in _STREAMS_DICT will be added

@andersy005
Copy link
Member

@mgrover1,

I am in favor of Mike's suggestion. It is reasonable to treat this operation as a dictionary update in which we give priority to the user provided values.

@mgrover1
Copy link
Contributor Author

@mnlevy1981 thanks for pointing that out - that sounds like a great way to move forward!

@mgrover1
Copy link
Contributor Author

@andersy005 would it be best to change https://github.com/NCAR/ecgtools/blob/main/ecgtools/builder.py#L140 to take in arguments regarding the file parser? Or is there a way to do this at the file parser level where one could "update" the file parser?

Copy link
Member

@andersy005 andersy005 left a comment

Choose a reason for hiding this comment

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

Thank you, Max! This looks great!

@andersy005 andersy005 merged commit 762e005 into ncar-xdev:main Jun 18, 2021
@andersy005 andersy005 added api_change Breaking change enhancement New feature or request labels Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api_change Breaking change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants