Dev

[granthutchings]

No description provided.

[luiarthur]

Great work, @granthutchings!

I left some comments and suggestions. If any of them are too annoying, feel free to ignore.

Since I'm not familiar with the source and I don't know what the changes actually do, the changes I suggested mainly concern readability.

If possible, lines should be restricted to no longer than 90 characters in length. (If you are using a text editor like vim or vscode, there are options to enforce that of hard-wrap long lines.)

Possible ways for breaking down long lines include storing long function arguments in a variable before using them as function arguments.

To enhance readability, you can also rewrite long lines or sequence of operations as functions. (Be mindful, though, of declaring functions in functions that are used in time-critical code.

For example, I noticed some long function arguments are passed to some plot functions. They should probably be defined prior to the plt.plot function for better readability. (Please double check to make sure I didn't mess up in the suggestions.)

Also, small commits which provide one (maybe two) new features are usually preferred. In the case of rewriting tests, that's probably unreasonable. But, something to keep in mind.

NOTE: you can click commit suggestion where applicable in this review. In other places, you can just commit your changes if you want to address any of the comments.

[granthutchings]

Great work, @granthutchings!

I left some comments and suggestions. If any of them are too annoying, feel free to ignore.

Since I'm not familiar with the source and I don't know what the changes actually do, the changes I suggested mainly concern readability.

If possible, lines should be restricted to no longer than 90 characters in length. (If you are using a text editor like vim or vscode, there are options to enforce that of hard-wrap long lines.)

Possible ways for breaking down long lines include storing long function arguments in a variable before using them as function arguments.

To enhance readability, you can also rewrite long lines or sequence of operations as functions. (Be mindful, though, of declaring functions in functions that are used in time-critical code.

For example, I noticed some long function arguments are passed to some plot functions. They should probably be defined prior to the plt.plot function for better readability. (Please double check to make sure I didn't mess up in the suggestions.)

Also, small commits which provide one (maybe two) new features are usually preferred. In the case of rewriting tests, that's probably unreasonable. But, something to keep in mind.

NOTE: you can click commit suggestion where applicable in this review. In other places, you can just commit your changes if you want to address any of the comments.

Thanks @luiarthur, ill commit some revisions in the next couple of days.

[granthutchings]

@luiarthur revisions done in commits d11720f, d0992cd

[luiarthur]

Excellent, @granthutchings!

I just made a few formatting suggestions. If they look correct, then you can hit commit suggestions.

I'm fine with merging this unless anyone else wanted to take a look.

cc: @jgattiker @natalieklein229

[natalieklein229]

Apologies that I have not had a chance to look yet. I should be able to take a glance ths afternoon! From: Arthur Lui ***@***.***> Reply-To: lanl/SEPIA ***@***.***> Date: Tuesday, August 31, 2021 at 12:32 PM To: lanl/SEPIA ***@***.***> Cc: "Klein, Natalie Elizabeth" ***@***.***>, Mention ***@***.***> Subject: [EXTERNAL] Re: [lanl/SEPIA] Dev (#38) @luiarthur requested changes on this pull request. Excellent, @granthutchings<https://github.com/granthutchings>! I just made a few formatting suggestions. If they look correct, then you can hit commit suggestions. I'm fine with merging this unless anyone else wanted to take a look. cc: @jgattiker<https://github.com/jgattiker> @natalieklein229<https://github.com/natalieklein229>

________________________________ In sepia/SepiaPlot.py<#38 (comment)>:

@@ -755,62 +755,68 @@ def pca_projected_data(data):

# show data - observation and simulations plt.subplot(2,2,1) n_obs_lines = data.obs_data.y.T.shape[1] - plt.plot(data.sim_data.y_ind,data.sim_data.y.T) - plt.plot(data.obs_data.y_ind,data.obs_data.y.T,'k',linewidth=2,label=['observation']+['_']*(n_obs_lines-1) if n_obs_lines>1 else 'observation') + plt.plot(data.sim_data.y_ind, data.sim_data.y.T) + label = ['observation']+['_']*(n_obs_lines-1) if n_obs_lines>1 else 'observation' ⬇️ Suggested change - label = ['observation']+['_']*(n_obs_lines-1) if n_obs_lines>1 else 'observation' + label = ['observation'] + ['_'] * (n_obs_lines-1) if n_obs_lines > 1 else 'observation'

________________________________ In sepia/SepiaPlot.py<#38 (comment)>:

+ y_obs_pca = ***@***.***_data.K)*

+ data.obs_data.orig_y_sd + data.obs_data.orig_y_mean).T ⬇️ Suggested change - y_obs_pca = ***@***.***_data.K)* - data.obs_data.orig_y_sd + data.obs_data.orig_y_mean).T + y_obs_pca = sp.linalg.lstsq(data.obs_data.K.T,data.obs_data.y_std.T)[0].T @ data.obs_data.K + y_obs_pca = (y_obs_pca * data.obs_data.orig_y_sd + data.obs_data.orig_y_mean).T

________________________________ In sepia/SepiaPlot.py<#38 (comment)>:

+ y_sim_pca = ***@***.***_data.K)*

+ data.sim_data.orig_y_sd + data.sim_data.orig_y_mean).T ⬇️ Suggested change - y_sim_pca = ***@***.***_data.K)* - data.sim_data.orig_y_sd + data.sim_data.orig_y_mean).T + y_sim_pca = sp.linalg.lstsq(data.sim_data.K.T, data.sim_data.y_std.T)[0].T @ data.sim_data.K + y_sim_pca = (y_sim_pca * data.sim_data.orig_y_sd + data.sim_data.orig_y_mean).T

________________________________ In sepia/SepiaPlot.py<#38 (comment)>:

plt.legend()

plt.title('Data: sims, obs') # show obs and reconstructed obs alone plt.subplot(2,2,2) - plt.plot(data.obs_data.y_ind,data.obs_data.y.T,'k',linewidth=2,label=['observation']+['_']*(n_obs_lines-1) if n_obs_lines>1 else 'observation') - ***@***.***_data.K)* - data.obs_data.orig_y_sd + data.obs_data.orig_y_mean).T,'r--',linewidth=2, - label=['PCA modeled observation']+['_']*(n_obs_lines-1) if n_obs_lines>1 else 'PCA modeled observation') + y_obs_pca = ***@***.***_data.K)* + data.obs_data.orig_y_sd + data.obs_data.orig_y_mean).T + y_sim_pca = ***@***.***_data.K)* + data.sim_data.orig_y_sd + data.sim_data.orig_y_mean).T + label = ['observation']+['_']*(n_obs_lines-1) if n_obs_lines>1 else 'observation' + plt.plot(data.obs_data.y_ind, data.obs_data.y.T, 'k', linewidth=2, label=label) + label = ['PCA modeled observation']+['_']*(n_obs_lines-1) if n_obs_lines>1 else 'PCA modeled observation' ⬇️ Suggested change - label = ['PCA modeled observation']+['_']*(n_obs_lines-1) if n_obs_lines>1 else 'PCA modeled observation' + label = ['PCA modeled observation'] + ['_'] * (n_obs_lines - 1) if n_obs_lines > 1 else 'PCA modeled observation'

________________________________ In sepia/SepiaPlot.py<#38 (comment)>:

plt.legend()

plt.title('PCA truncation effect on observation') # show data projected and reconstructed through K basis # (this is the problem being solved given PCA truncation) plt.subplot(2,2,3) # add the obs projected and reconstructed through the K basis - ***@***.***_data.K)* - data.sim_data.orig_y_sd + data.sim_data.orig_y_mean).T) - ***@***.***_data.K)* - data.obs_data.orig_y_sd + data.obs_data.orig_y_mean).T,'k',linewidth=2, - label=['observation']+['_']*(n_obs_lines-1) if n_obs_lines>1 else 'observation') + plt.plot(data.sim_data.y_ind, y_sim_pca) + label = ['observation']+['_']*(n_obs_lines-1) if n_obs_lines>1 else 'observation' ⬇️ Suggested change - label = ['observation']+['_']*(n_obs_lines-1) if n_obs_lines>1 else 'observation' + label = ['observation'] + ['_'] * (n_obs_lines - 1) if n_obs_lines > 1 else 'observation'

________________________________ In sepia/SepiaPlot.py<#38 (comment)>:

plt.legend()

plt.title('Data: sims, obs') # show obs and reconstructed obs alone plt.subplot(2,2,2) + y_obs_pca = ***@***.***_data.K[i])* + data.obs_data.orig_y_sd[i] + data.obs_data.orig_y_mean[i]).T for i in range(n_y_obs)] ⬇️ Suggested change - data.obs_data.orig_y_sd[i] + data.obs_data.orig_y_mean[i]).T for i in range(n_y_obs)] + data.obs_data.orig_y_sd[i] + data.obs_data.orig_y_mean[i]).T for i in range(n_y_obs)]

________________________________ In sepia/SepiaPlot.py<#38 (comment)>:

plt.legend()

plt.title('PCA truncation effect on observation') # show data projected and reconstructed through K basis # (this is the problem being solved given PCA truncation) plt.subplot(2,2,3) # add the obs projected and reconstructed through the K basis - ***@***.***_data.K)* - data.sim_data.orig_y_sd + data.sim_data.orig_y_mean).T) + y_sim_pca = ***@***.***_data.K)* + data.sim_data.orig_y_sd + data.sim_data.orig_y_mean).T ⬇️ Suggested change - data.sim_data.orig_y_sd + data.sim_data.orig_y_mean).T + data.sim_data.orig_y_sd + data.sim_data.orig_y_mean).T

________________________________ In test/test_Neddermeyer.py<#38 (comment)>:

+ y_ind_obs_1 = np.column_stack( ( np.concatenate((np.ones(phi_obs.shape[0])*time_obs[0][0],\

+ np.ones(phi_obs.shape[0])*time_obs[0][1],\ + np.ones(phi_obs.shape[0])*time_obs[0][2])), np.tile(phi_obs,3).T ) ) ⬇️ Suggested change - y_ind_obs_1 = np.column_stack( ( np.concatenate((np.ones(phi_obs.shape[0])*time_obs[0][0],\ - np.ones(phi_obs.shape[0])*time_obs[0][1],\ - np.ones(phi_obs.shape[0])*time_obs[0][2])), np.tile(phi_obs,3).T ) ) + y_ind_obs_1 = np.column_stack([ + np.concatenate((np.ones(phi_obs.shape[0])*time_obs[0][0], + np.ones(phi_obs.shape[0])*time_obs[0][1], + np.ones(phi_obs.shape[0])*time_obs[0][2])), + np.tile(phi_obs, 3).T + ])

________________________________ In test/test_Neddermeyer.py<#38 (comment)>:

y_obs[i] = (y_obs[i] + .01*np.random.normal(size=y_obs[i].shape)).flatten()

# indices of observations x_obs = ((np.array([params10[4], .17, .36])/.32-.5)/.65).reshape(3,1) - y_ind_obs = [np.column_stack( ( np.concatenate((np.ones(phi_obs.shape[0])*time_obs[0][0], np.ones(phi_obs.shape[0])*time_obs[0][1], np.ones(phi_obs.shape[0])*time_obs[0][2])), np.tile(phi_obs,3).T ) ), - np.column_stack( ( (np.ones(phi_obs.shape[0])*time_obs[1]).reshape(16,1), phi_obs.T ) ), - np.column_stack( ( np.concatenate((np.ones(phi_obs.shape[0])*time_obs[2][0],\ - np.ones(phi_obs.shape[0])*time_obs[2][1])), np.tile(phi_obs,2).T ) )] - + # create y_ind_obs where each row is a (time, angle) pair. Pairs are grouped by time. + # experiment 1 + y_ind_obs_1 = np.column_stack( ( np.concatenate((np.ones(phi_obs.shape[0])*time_obs[0][0],\ + np.ones(phi_obs.shape[0])*time_obs[0][1],\ + np.ones(phi_obs.shape[0])*time_obs[0][2])), np.tile(phi_obs,3).T ) ) + # experiment 2 + y_ind_obs_2 = np.column_stack( ( (np.ones(phi_obs.shape[0])*time_obs[1]).reshape(16,1), phi_obs.T ) ) ⬇️ Suggested change - y_ind_obs_2 = np.column_stack( ( (np.ones(phi_obs.shape[0])*time_obs[1]).reshape(16,1), phi_obs.T ) ) + y_ind_obs_2 = np.column_stack([ + (np.ones(phi_obs.shape[0]) * time_obs[1]).reshape(16,1), + phi_obs.T + ])

________________________________ In test/test_Neddermeyer.py<#38 (comment)>:

+ y_ind_obs_3 = np.column_stack( ( np.concatenate((np.ones(phi_obs.shape[0])*time_obs[2][0],\

+ np.ones(phi_obs.shape[0])*time_obs[2][1])), np.tile(phi_obs,2).T ) ) ⬇️ Suggested change - y_ind_obs_3 = np.column_stack( ( np.concatenate((np.ones(phi_obs.shape[0])*time_obs[2][0],\ - np.ones(phi_obs.shape[0])*time_obs[2][1])), np.tile(phi_obs,2).T ) ) + y_ind_obs_3 = np.column_stack([ + np.concatenate([ + np.ones(phi_obs.shape[0]) * time_obs[2][0], + np.ones(phi_obs.shape[0]) * time_obs[2][1] + ]), + np.tile(phi_obs, 2).T + ])

________________________________ In sepia/SepiaPlot.py<#38 (comment)>:

plt.legend()

plt.title('PCA truncation effect on observation') # show data projected and reconstructed through K basis # (this is the problem being solved given PCA truncation) plt.subplot(2,2,3) # add the obs projected and reconstructed through the K basis - ***@***.***_data.K)* - data.sim_data.orig_y_sd + data.sim_data.orig_y_mean).T) + y_sim_pca = ***@***.***_data.K)* + data.sim_data.orig_y_sd + data.sim_data.orig_y_mean).T Added a few more spaces to align with parenthesis. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#38 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AQBKU4UVB7R6S5DJLIGED53T7UN35ANCNFSM5C5UV6KQ>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[natalieklein229]

Looks good! I think the separation of tests and notebooks is great.

It looks like the documentation could probably use a little updating; I can try to look at that, or please feel free to take a stab at it by editing docs/*.rst files.

[luiarthur]

Great, I think this is ready to be merged.

@jgattiker, if you feel good about the changes, you approve the changes.

This PR has some merge conflicts, but I can resolve those and merge after.

Might need @granthutchings's help if I run into anything tricky.

[jgattiker]

cool

[luiarthur]

I'll merge this in the morning.

[luiarthur]

Thanks @granthutchings for this PR. And thanks @natalieklein229 and @jgattiker for reviewing.

FYI, I've squash-merged dev into master, and deleted dev.

Let me know if you have any questions.