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

Develop minimization plots #85

Merged
merged 13 commits into from
Nov 15, 2021
Merged

Conversation

kevindougherty-noaa
Copy link
Collaborator

Addresses 1.) in #84. Creates two minimization plots for a single cycle: Jo and gradnorm
Jo:
image
Gradnorm:
image

@kevindougherty-noaa kevindougherty-noaa changed the base branch from develop to feature/LAMDA October 13, 2021 19:35
@kevindougherty-noaa kevindougherty-noaa marked this pull request as ready for review November 12, 2021 22:44
Copy link
Contributor

@CoryMartin-NOAA CoryMartin-NOAA left a comment

Choose a reason for hiding this comment

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

Couple of quick comments/suggestions but mostly looks great

"""
# Get cycle info
cycle = df.index.get_level_values(0)[-1]
cyclestr = datetime.strftime(cycle, '%Y%m%d%H')
Copy link
Contributor

Choose a reason for hiding this comment

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

Only question is how to handle the tm06 information for this plot?

myplot.add_ylabel('log (J)')
myplot.add_legend(loc='upper right',
fontsize='large')
myplot.add_title('FV3LAM Cost', loc='left')
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there are multiple experiments (FV3LAMDA, LAMDAX, etc.) can we make the title include an optional experiment name passed to the function?

plt.close('all')


def _plot_gnorm(df, outdir):
Copy link
Contributor

Choose a reason for hiding this comment

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

same two comments for this function as the first one.

Copy link
Contributor

@CoryMartin-NOAA CoryMartin-NOAA left a comment

Choose a reason for hiding this comment

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

Quick comments, almost ready to merge!

_plot_gnorm(df, outdir)
_plot_cost_function(df, outdir)
_plot_gnorm(df, plotting_config, outdir)
_plot_cost_function(df, plotting_config, outdir)
Copy link
Contributor

Choose a reason for hiding this comment

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

weird no end of line thing here again like what happens sometimes

plotting_config = {}
plotting_config['tm'] = tm
current_file = fits2_data[-1].split('/')[-1]
plotting_config['experiment'] = current_file.split('.')[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this return? Ideally experiment would be a YAML defined thing I think.

Copy link
Contributor

@CoryMartin-NOAA CoryMartin-NOAA left a comment

Choose a reason for hiding this comment

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

Real quick

@@ -99,13 +99,15 @@ def plotting(config, data_dict, outdir, data_type, ob_type):
plot_dict[plot_type](fits_df, plotting_config, outdir)


def create_minimization_plots(data_dict, outdir):
def create_minimization_plots(data_dict, experiment_type, outdir):
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 nitpicking but can you change this to experiment_name? Once we have something running, someone might run an experiment to test something new like my_new_radiosonde_test or use_more_data and not just one of 'LAMDA, LAMDAX, etc.'. Then I will approve.

@CoryMartin-NOAA CoryMartin-NOAA merged commit abb468c into feature/LAMDA Nov 15, 2021
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