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

Accessing GSI observing system records for active and available channels #273

Merged
merged 38 commits into from
Nov 15, 2023

Conversation

asewnath
Copy link
Contributor

@asewnath asewnath commented Oct 30, 2023

Description

Initial PR making changes required to update yaml use flags during cylc workflow run. PR does the following:

  • Change amsua_n19.yaml to include active channel jinja templating in use_flag field
  • Adding two new tasks: CloneGeosAna and GenerateObservingSystemRecords
  • Created new object ObservingSystemRecords to handle parsing active and available channel tables from GSI
  • Modified render_interface_observation function to call new get_active_channels function.
  • Modifying relevant tasks to get observing_system_record_path for render_interface_observation or jedi_dictionary_iterator function

Changes that still need to be made: (can be in this PR or another one)

  • Update all yaml files with use_flag field to have active channel jinja templating
  • Available channels jinja templating in all yaml files
  • Add logger into functions for proper aborts

Copy link
Contributor

@danholdaway danholdaway 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 very much for adding this very important functionality @asewnath. I just have a few minor comments about code readability. The overall functionality looks great.

The only other comment I would make is in having these three files: sat_db_utils, instr_state_machine and observing_system_records. Do we still need all three? What is the difference between what's in the files? It's not very easy to tell by the names of the files.

src/swell/suites/ufo_testing/flow.cylc Outdated Show resolved Hide resolved
src/swell/suites/ufo_testing/flow.cylc Outdated Show resolved Hide resolved
src/swell/tasks/base/task_registry.py Outdated Show resolved Hide resolved
src/swell/tasks/gsi_bc_to_ioda.py Outdated Show resolved Hide resolved
src/swell/tasks/task_questions.yaml Outdated Show resolved Hide resolved
src/swell/utilities/render_jedi_interface_files.py Outdated Show resolved Hide resolved
src/swell/utilities/render_jedi_interface_files.py Outdated Show resolved Hide resolved
src/swell/utilities/run_jedi_executables.py Outdated Show resolved Hide resolved
@asewnath
Copy link
Contributor Author

asewnath commented Oct 31, 2023

Thank you very much for adding this very important functionality @asewnath. I just have a few minor comments about code readability. The overall functionality looks great.

The only other comment I would make is in having these three files: sat_db_utils, instr_state_machine and observing_system_records. Do we still need all three? What is the difference between what's in the files? It's not very easy to tell by the names of the files.

The observing_system_records file contains the ObservingSystemRecords object, which has methods to parse the GSI records and save the yaml files.instr_state_machine is an old name that can be changed to something like gsi_tbl_parser. It contains the code that actually parses the GSI records. Since it's a lot of code, it seems cleaner to have it in a separate file. The sat_db_utils file contains different utilities for the channel update process. read_sat_db is used only by ObservingSystemRecords and can be moved to observing_system_records. run_sat_db_process is legacy code that can be deleted. The only place it is called in is utilities/scripts/swell_sat_db_processing.py, which can also be deleted. Then there are functions that are used for the channel update that are not associated with the ObservingSystemRecords object. This includes get_active_channels and other functions necessary for the active channel retrieval. A separate file seems appropriate for these functions, and the file itself can be changed to get_active_channels or something along those lines. I can also add more documentation to all these files.

@danholdaway
Copy link
Contributor

Thank you very much for adding this very important functionality @asewnath. I just have a few minor comments about code readability. The overall functionality looks great.
The only other comment I would make is in having these three files: sat_db_utils, instr_state_machine and observing_system_records. Do we still need all three? What is the difference between what's in the files? It's not very easy to tell by the names of the files.

The observing_system_records file contains the ObservingSystemRecords object, which has methods to parse the GSI records and save the yaml files.instr_state_machine is an old name that can be changed to something like gsi_tbl_parser. It contains the code that actually parses the GSI records. Since it's a lot of code, it seems cleaner to have it in a separate file. The sat_db_utils file contains different utilities for the channel update process. read_sat_db is used only by ObservingSystemRecords and can be moved to observing_system_records. run_sat_db_process is legacy code that can be deleted. The only place it is called in is utilities/scripts/swell_sat_db_processing.py, which can also be deleted. Then there are functions that are used for the channel update that are not associated with the ObservingSystemRecords object. This includes get_active_channels and other functions necessary for the active channel retrieval. A separate file seems appropriate for these functions, and the file itself can be changed to get_active_channels or something along those lines. I can also add more documentation to all these files.

Thanks for the explanation. I think some renaming and clean up of any unused code should be sufficient.

@danholdaway danholdaway merged commit 90129ec into develop Nov 15, 2023
9 checks passed
@danholdaway danholdaway deleted the feature/use_satbd_for_channel_selection branch November 15, 2023 21:03
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