Version 1.0: North Atlantic Update - Tides, Curved Boundaries, Config, and More!😀🤯🥳 #193
Conversation
…CESM when parsing
Override min depth
Changes to make compatible with CESM
having mask table and layout no longer required
Fix layout requirements
fix typo in find replace
Replace minimum layers with min depth
…nt num After talking with Ashley, the find_MOM6_orientation will likely be removed
The function names for rectangular and sinmple boundaries were changed because the tides are a kind of boundary function, the old names now give a warning and call the correct function. GFDL had rough horizontal subsetting for the tpxo dataset (probably for efficiency?), implemented in setup_tides_rectangular_boundaries.
Collapsed the *_tidal_dims functions into the encode_tides function in segment, and add citing documentation to the docstrings for now.
…l-mom6 into croc_with_req_changes
Add angled grids to RMOM6
Update comment to reflect name change
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
manishvenu
changed the title
|
Hi @manishvenu T H A N K S ! This is a huge PR. Is there any chance we break it into a few smaller ones? Otherwise reviewing will be hard.
|
| @@ -129,7 +129,7 @@ def test_quadrilateral_areas(lat, lon, true_area): | |||
| ], | |||
| ) | |||
| def test_rectangular_hgrid(lat, lon): | |||
| assert isinstance(rectangular_hgrid(lat, lon), xr.Dataset) | |||
| assert isinstance(generate_rectangular_hgrid(lat, lon), xr.Dataset) | |||
There was a problem hiding this comment.
The args are reversed in the test compared to the method definition
Probably doesn't matter for this test, but it might create confusion to users/developers.
Yes it's a huge PR! We did have this broken down into smaller PRs on the CROCODILE fork, which we reviewed and merged one by one over the last couple of months. Unfortunately these PRs might not be useful for us to break up this big one any more, because we did need to do a lot of merging given that @manishvenu and I were working in parallel on several changes. It's still in draft state for now while we polish it (build on the Is there a way you suggest that we break up the PR? Since we did a huge overhaul of the entire codebase, it's kind of hard to separate some of the changes! |
…up_run_directory (#20) * Package type changes for CRR * Black Formatting * Change MOM Param * Change MOM_param to Work * black * Remvoe premade_run_dir_arg * Remove init * Black * Update MOM_param_test * Clean
* include DataReader class * within __getattr__ read data files * bug * update demo with plots * typo
OK, when is ready to review we can talk again? Huge PRs are harder to review though and take longer...
Each of the major features mentioned in the first comment sounds like could be a PR? Just a suggestion.. |
haha, sorry if I introduced this difficulty to devs |
…x __getattr__ bug (#24)
… + tests (#30) * Package type changes for CRR * Black Formatting * Change MOM Param * Change MOM_param to Work * black * Revert back to reg * Add tide boundary flexibility * Update Config to only vars * Update Write and Read Config for computer independence * Update Read/Write Config for Computer Independenc * Fix Change MOM Param Issues * Fix bug in __getattr__ and comment out matplotlib dependencies in demo * Package type changes for CRR * Black Formatting * Add Boundaries to Init part 1 * black formatting * Changing Boundary Def to __init__ pt 2 * Minor Bugs * Make Direction Dir a one-time calculation because keys() doesn't gaurentee order * Rearrange handling boundaries in __init__ to a boundary array and segments dict in the boundary functions ocean_state and tides * Clean * Improve commenting on setuop_ocean_state_boundaries * Fix bug in declaring number of OBC segments * Minor Big in override * Add boundaries to config
| except: | ||
| raise ValueError( | ||
| "Invalid Input. Tidal constituent {} is not supported.".format(tc) | ||
| ) |
There was a problem hiding this comment.
There'll be a few of these, but catch only the exception you're looking for here. I guess in this case it's a KeyError?
| list_of_ints = [] | ||
| for tc in tidal_constituents: | ||
| try: | ||
| list_of_ints.append(tidal_constituents_tpxo_dict[tc]) | ||
| except: | ||
| raise ValueError( | ||
| "Invalid Input. Tidal constituent {} is not supported.".format(tc) | ||
| ) | ||
|
|
||
| return list_of_ints |
There was a problem hiding this comment.
| list_of_ints = [] | |
| for tc in tidal_constituents: | |
| try: | |
| list_of_ints.append(tidal_constituents_tpxo_dict[tc]) | |
| except: | |
| raise ValueError( | |
| "Invalid Input. Tidal constituent {} is not supported.".format(tc) | |
| ) | |
| return list_of_ints | |
| try: | |
| constituent_indices = [constituent_map[tc] for tc in tidal_constituents] | |
| except KeyError as e: | |
| raise ValueError(f"Invalid tidal constituent: {e.args[0]}") | |
| return constituent_indices |
| try: | ||
| expt.longitude_extent = tuple(config_dict["longitude_extent"]) | ||
| expt.latitude_extent = tuple(config_dict["latitude_extent"]) | ||
| except: | ||
| expt.longitude_extent = None | ||
| expt.latitude_extent = None |
There was a problem hiding this comment.
| try: | |
| expt.longitude_extent = tuple(config_dict["longitude_extent"]) | |
| expt.latitude_extent = tuple(config_dict["latitude_extent"]) | |
| except: | |
| expt.longitude_extent = None | |
| expt.latitude_extent = None | |
| try: | |
| expt.longitude_extent = tuple(config_dict["longitude_extent"]) | |
| expt.latitude_extent = tuple(config_dict["latitude_extent"]) | |
| except KeyError: | |
| expt.longitude_extent = None | |
| expt.latitude_extent = None |
Check that extents are correct length?
| try: | ||
| expt.date_range = config_dict["date_range"] | ||
| expt.date_range[0] = dt.datetime.strptime( | ||
| expt.date_range[0], "%Y-%m-%d %H:%M:%S" | ||
| ) | ||
| expt.date_range[1] = dt.datetime.strptime( | ||
| expt.date_range[1], "%Y-%m-%d %H:%M:%S" | ||
| ) | ||
| except: | ||
| expt.date_range = None |
There was a problem hiding this comment.
Maybe KeyError for the path where expt.date_range = None, but if you handle IndexError separately you can warn/error on the incorrect range input.
| if mom_input_folder is None: | ||
| mom_input_folder = Path(os.path.join("mom_run", "from_config")) | ||
| if mom_run_folder is None: | ||
| mom_run_folder = Path(os.path.join("mom_input", "from_config")) |
There was a problem hiding this comment.
| if mom_input_folder is None: | |
| mom_input_folder = Path(os.path.join("mom_run", "from_config")) | |
| if mom_run_folder is None: | |
| mom_run_folder = Path(os.path.join("mom_input", "from_config")) | |
| if mom_input_folder is None: | |
| mom_input_folder = Path("mom_run" / "from_config") | |
| if mom_run_folder is None: | |
| mom_run_folder = Path("mom_input" / "from_config") |
or just e.g. "mom_input/from_config"
| if seg == "south": | ||
| index_str = '"J=0,I=0:N' | ||
| elif seg == "north": | ||
| index_str = '"J=N,I=N:0' | ||
| elif seg == "west": | ||
| index_str = '"I=0,J=N:0' | ||
| elif seg == "east": | ||
| index_str = '"I=N,J=0:N' |
There was a problem hiding this comment.
Would be a little cleaner as a dictionary
| key_DATA = key_start + "_DATA" | ||
| file_num_obc = str( | ||
| self.find_MOM6_rectangular_orientation(seg) | ||
| ) # 1,2,3,4 for rectangular boundaries, BUT if we have less than 4 segments we use the index to specific the number, but keep filenames as if we had four boundaries | ||
| MOM_override_dict[key_DATA][ | ||
| "value" | ||
| ] = f'"U=file:forcing_obc_segment_00{file_num_obc}.nc(u),V=file:forcing_obc_segment_00{file_num_obc}.nc(v),SSH=file:forcing_obc_segment_00{file_num_obc}.nc(eta),TEMP=file:forcing_obc_segment_00{file_num_obc}.nc(temp),SALT=file:forcing_obc_segment_00{file_num_obc}.nc(salt)' | ||
| if with_tides: | ||
| MOM_override_dict[key_DATA]["value"] = ( | ||
| MOM_override_dict[key_DATA]["value"] | ||
| + f',Uamp=file:tu_segment_00{file_num_obc}.nc(uamp),Uphase=file:tu_segment_00{file_num_obc}.nc(uphase),Vamp=file:tu_segment_00{file_num_obc}.nc(vamp),Vphase=file:tu_segment_00{file_num_obc}.nc(vphase),SSHamp=file:tz_segment_00{file_num_obc}.nc(zamp),SSHphase=file:tz_segment_00{file_num_obc}.nc(zphase)"' | ||
| ) | ||
| else: | ||
| MOM_override_dict[key_DATA]["value"] = ( | ||
| MOM_override_dict[key_DATA]["value"] + '"' | ||
| ) |
There was a problem hiding this comment.
Would be a bit clearer to build up the value string outside of the dictionary, then only set it at the end.
Should also split the really long lines, i.e.
obc_string = (
f",Uamp=file:tu_segment_00{file_num_obc}.nc(uamp),"
f"Uphase=file:tu_segment_00{file_num_obc}.nc(uphase),"
f"Vamp=file:tu_segment_00{file_num_obc}.nc(vamp),"
f"Vphase=file:tu_segment_00{file_num_obc}.nc(vphase),"
f"SSHamp=file:tz_segment_00{file_num_obc}.nc(zamp),"
f"SSHphase=file:tz_segment_00{file_num_obc}.nc(zphase)"'
)| str(self.date_range[0].year) | ||
| + ", " | ||
| + str(self.date_range[0].month) | ||
| + ", " | ||
| + str(self.date_range[0].day) | ||
| ) |
| for key in MOM_override_dict.keys(): | ||
| if type(MOM_override_dict[key]) == dict: | ||
| MOM_override_dict[key]["override"] = True |
There was a problem hiding this comment.
| for key in MOM_override_dict.keys(): | |
| if type(MOM_override_dict[key]) == dict: | |
| MOM_override_dict[key]["override"] = True | |
| for key, val in MOM_override_dict.items(): | |
| if isinstance(val, dict): | |
| MOM_override_dict[key]["override"] = True |
| @@ -1754,6 +2504,198 @@ def setup_run_directory( | |||
| 0, | |||
| ] | |||
| nml.write(self.mom_run_dir / "input.nml", force=True) | |||
| return | |||
|
|
|||
| def change_MOM_parameter( | |||
There was a problem hiding this comment.
I think this MOM parameter file stuff could get lifted out to a separate file?
Authors: @ashjbarnes, @manishvenu
Description:
Ashley and I worked together to add a few features to RM6. While we get the changes ready to merge with the main COSIMA branch, please check out this draft with a list of the changes below.
Major Features:
Minor Features:
Breaking Changes (for backwards compatibility):
Documentation Changes (TBD):
Co-authored-by: @ashjbarnes
this PR closes #184 , #168 , and also #55 in a roundabout way since user now specifies which boundaries they want, and the MOM inputs are adjusted accordingly.
Before merging, need to resolve the following: