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

Split Gulf Steam plot from ssh_eval and rewrite ssh_eval to use config file #101

Merged
merged 9 commits into from
Oct 28, 2024

Conversation

uwagura
Copy link
Collaborator

@uwagura uwagura commented Oct 18, 2024

This PR separates the gulf stream position and index plot form the mean ssh plot, and generalizes the ssh plot to use the config.yaml file. Plots included below; the arctic and MHI plots were made using only about 2 weeks worth of data, so as always don't read into the metrics or plots too deeply, and please forgive the sloppy formatting!

It looks like the cartopy.axes.set_xticks function that add_ticks calls does not yet support non-rectangular projections, so as a work around for the arctic I included an if statement before this function is called that checks if the grid is in the NorthPolarStereo projection before calling add_ticks. This workaround feels a bit clumsy, but should be good for the projections we expect right now. Would appreciate people's thoughts on whether or not there is a better way to do this check.

mean_ssh_eval_nwa
gulfstream_eval
mean_ssh_eval_arctic
mean_ssh_eval_MHI
mean_ssh_eval_nep

save_figure('gulfstream_eval', label=label, pdf=True)
# Set projection of each grid in the plot
# For now, sst_eval.py will only support a projection for the arctic and a projection for all other domains
if config['projection_grid'] == 'NorthPolarStereo':
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but in other scripts (e.g., sst_eval.py), you used p for the grid projection:

if config['projection_grid'] == 'NorthPolarStereo':
p = ccrs.NorthPolarStereo()
else:
p = ccrs.PlateCarree()

It might be a good idea to make the naming consistent with similar scripts. I understand that if you do this, you'll also have to modify the following:
p = grid[0].pcolormesh(model_grid.geolon_c, model_grid.geolat_c, model_ssh_ave, cmap=cmap, norm=norm, transform = proj )
cbar = autoextend_colorbar(grid.cbar_axes[0], p)
and
p = grid[1].pcolormesh(glorys_lonc, glorys_latc, glorys_zos_ave, cmap=cmap, norm=norm, transform = proj )
cbar = autoextend_colorbar(grid.cbar_axes[1], p)
too.

projection = proj
)
else:
logger.warning("WARNING: Cannot set tick marks in non-rectangular projection. Will not call add_ticks")
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 okay with this approach. If we do want to add ticks to the NorthPolarStereo projection plot, we can try using Gridliner:

        # Add Gridliner for NorthPolarStereo projection
        gridliner = ax.gridlines(draw_labels=True, color='gray', linestyle='--')
        gridliner.xlabels_top = False  # Disable top labels
        gridliner.ylabels_right = False  # Disable right labels
        gridliner.xformatter = ccrs.cartopy.mpl.ticker.LongitudeFormatter()
        gridliner.yformatter = ccrs.cartopy.mpl.ticker.LatitudeFormatter()
        gridliner.xlabel_style = {'size': 10, 'color': 'gray'}
        gridliner.ylabel_style = {'size': 10, 'color': 'gray'}
        gridliner.xlocator = plt.FixedLocator([-180, -90, 0, 90, 180])  # Set longitudes
        gridliner.ylocator = plt.FixedLocator([60, 70, 80, 90])  # Set latitudes

…pts, Added noted about lat/lon consistency
@uwagura uwagura requested a review from yichengt900 October 24, 2024 15:12
Copy link
Contributor

@yichengt900 yichengt900 left a comment

Choose a reason for hiding this comment

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

@uwagura, thanks for the updates! Please also consider renaming line 95 from p to p0:

p = grid[0].pcolormesh(model_grid.geolon_c, model_grid.geolat_c, model_ssh_ave, cmap=cmap, norm=norm, transform = proj )

and line 102 fromp to p1:

p = grid[1].pcolormesh(glorys_lonc, glorys_latc, glorys_zos_ave, cmap=cmap, norm=norm, transform = proj )

Again it's not a big issue, but it would be good to maintain consistency with the other scripts.

Also, please consider moving plot_gulf_stream.py into the NWA12 subfolder, as it is specific to the NWA domain. Make sure to modify the code accordingly to ensure it still works correctly inside the NWA12 folder.

@uwagura uwagura requested a review from yichengt900 October 25, 2024 15:40
Copy link
Contributor

@yichengt900 yichengt900 left a comment

Choose a reason for hiding this comment

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

@uwagura, I found a few minor bugs in your PR that were preventing ssh_eval.py from running successfully. I’ve fixed these in a new commit—please review it to ensure it aligns with your intentions for this PR. The Gulf Stream script might benefit from additional refinement, but we can address that in a separate PR. I also noticed that the metric differs from the values in the paper, it is due to the different mask we're using. I’m fine with our current approach, and we can revisit this by adding an optional mask to the config file if needed.

@uwagura
Copy link
Collaborator Author

uwagura commented Oct 28, 2024

@yichengt900 Thanks for the commit. Regarding the changes:

1.) I purposefully edited the gulf stream plot to output figures into the NWA12 directory, because I thought it would be more intuitive for users to find them there than the figures directory in the parent folder. If you think the figures folder is still a better location, we can remove the output_dir argument from the save_figure plot as well.

2.) I had left a comment in ssh_eval about the longitude coordinates in the config file not matching the longitude coordinates range in model_grid.geolon, and reminding users to make sure these values align in their own model grid / config files. I decided to leave the responsibility of making sure these values use the same range to the user so that they could make sure all their files were consistent across all scripts. Will all grid files will use the [-180,180] range for longitude? if so, we can leave your edit and erase my comment.

@yichengt900
Copy link
Contributor

@yichengt900 Thanks for the commit. Regarding the changes:

1.) I purposefully edited the gulf stream plot to output figures into the NWA12 directory, because I thought it would be more intuitive for users to find them there than the figures directory in the parent folder. If you think the figures folder is still a better location, we can remove the output_dir argument from the save_figure plot as well.

2.) I had left a comment in ssh_eval about the longitude coordinates in the config file not matching the longitude coordinates range in model_grid.geolon, and reminding users to make sure these values align in their own model grid / config files. I decided to leave the responsibility of making sure these values use the same range to the user so that they could make sure all their files were consistent across all scripts. Will all grid files will use the [-180,180] range for longitude? if so, we can leave your edit and erase my comment.

@uwagura :
1.) For now, let’s keep all plot outputs in the figures folder. When we have specific plots for each domain, we can revisit and decide on the best organization.

2.) I saw your comment in the code. The issue is that if someone runs the script and config.yaml as-is, they’ll end up with NaN values in the metrics, which isn’t ideal. I’m not fully satisfied with my workaround either, and I don’t have a better solution at the moment—perhaps adding a mask section in the YAML could help. For now, let’s keep your comment, add a TODO note about the current trick we used to revisit later, and address it in a future update.

@uwagura uwagura requested a review from yichengt900 October 28, 2024 15:43
Conflicts:
	diagnostics/physics/config.yaml
@yichengt900 yichengt900 merged commit c313f29 into main Oct 28, 2024
2 checks passed
@yichengt900 yichengt900 deleted the split_ssh branch October 30, 2024 18:04
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