Scrutineering review #2

rebeccamccabe · 2022-11-01T04:07:28Z

I created this PR with a bit of convoluted branching according to the following instructions. https://thib.me/recipe-code-reviews-for-existing-code-with-github

Supplemental material for "Integrated Pumped Hydro Reverse Osmosis System Optimization Featuring Surrogate Model Development in Reverse Osmosis Modeling"

rebeccamccabe

I didn't read the equations closely yet, these comments just address code structure best practice recommendations

rebeccamccabe · 2022-11-01T04:15:09Z

Haefner_IPHROS_MOGA.m

+% Objective Function
+function J = objective(x)
+    % General Parameters
+    g = 9.81;             % gravitational acceleration [m/s^2]


Rather than repeating these parameters in both the objective and the constraint (and thereby introducing the possibility of inconsistency), it is preferred code practice to pass the parameters as a struct into both functions. See MDOcean as example.

rebeccamccabe · 2022-11-01T04:22:12Z

Haefner_IPHROS_MOGA.m

+    [E_rp,E_rd] = initial_energy_division(E_r,gamma);
+    V_wp = pump(E_rp,h,eta_hp,rho_sw,g);
+    [V_wRO, V_swht, Q_f] = mountaintop_reservoir(V_wp,gamma_RO,N_pv1);
+    [V_fwRO,V_oRO,S_oRO,rho_oRO,eta_ROio,eta_RO,rr_max,rr_min,Q_f_max,Q_f_min,Q_c_max,Q_c_min,Q_p_max,Q_p_min,LHS_SVM] = ro_system(h,Q_f,N_e1,N_e2,N_pv1,N_pv2,rho_sw,rho_fw,T,S_sw,M_salt,P_p,R,MW_salt_avg,Am);


This does not matter for GA, but keep in mind that when you move to gradient based optimization, you can achieve a ~2x speedup by running the simulation just once instead of twice for the objective and the constraint. Instructions here: https://www.mathworks.com/help/optim/ug/objective-and-nonlinear-constraints-in-the-same-function.html

rebeccamccabe · 2022-11-01T04:27:38Z

Haefner_IPHROS_MOGA.m

+    end
+
+    % Collection Lists
+    Q_p_collection = []; % gpd


Preferred coding practice is to initialize these lists with zeros(Ne1,1) (rather than initializing as empty and then continually enlarging the array). This is to avoid reallocating memory every time, and will probably give you a speedup.

rebeccamccabe · 2022-11-01T04:38:50Z

Haefner_IPHROS_MOGA.m

+
+    % Second stage
+    if N_e2 == 0 || N_pv2 == 0 || violation == 1 || violation_s1 == 1
+        i2 = N_e2+1;


if the intent of this is to drop out of the loop, then break is more appropriate than hardcoding the index.

rebeccamccabe · 2022-11-01T04:40:57Z

Haefner_IPHROS_MOGA.m

+        S_f_array = [S_f_collection;S_f_collection2];
+        Q_f_array = [Q_f_collection;Q_f_collection2];
+        LHS_SVM = -1e6;
+        for i = 1:length(P_f_array)


Vectorize this

rebeccamccabe · 2022-11-01T04:58:56Z

Haefner_IPHROS_MOGA.m

+            violation_s2 = 1;
+        else
+            % Storing element outputs for further calculations
+            Q_p_collection = [Q_p_collection;Q_p_num]; % gpd


Why do you store the Pf, Sf, and Qf collection always but the other variables you only store the collection if they are valid? This seems like it could potentially cause index inconsistencies

rebeccamccabe · 2022-11-02T00:40:06Z

Haefner_IPHROS_MOGA.m

+end
+
+% Miscellaneous Functions
+function S_brine = salinity_from_osmotic(pi,rho_fw,T,R,MW_salt_avg)


I would recommend solving the equation symbolically just once and then using matlabfunction to write the solution to a file where the solution can be evaluated numerically every time, rather than solving symbolically every time. This will speed things up quite drastically; I did a quick profile of the code and 80% of the runtime is just spent on the symbolic solver.

See for example https://github.com/symbiotic-engineering/MDOcean/tree/main/simulation/modules/symbolic

rebeccamccabe · 2022-11-02T00:45:07Z

Haefner_R_Eqn_Formulation.m

+coeff_rat = table2array(mdl_rat.Coefficients(:,1));
+
+% Data for plotting the fits
+for i = 1:1001


vectorize this

rebeccamccabe · 2022-11-02T00:45:51Z

Haefner_R_Eqn_Formulation.m

+new_coeff_power = table2array(new_mdl_power.Coefficients(:,1));
+
+% Data for plotting the fits
+for i = 1:1001


rebeccamccabe · 2022-11-02T00:46:17Z

Haefner_R_Eqn_Formulation.m

+
+% Getting Cook's distances, removing points
+cook_d = table2array(mdl_power.Diagnostics(:,2));
+yes_no = cook_d <= (4/2507);


why is 4/2507 the threshold?

rebeccamccabe · 2022-11-02T01:22:04Z

Haefner_IPHROS_MOGA.m

+    M_salt = 58.44;       % molar mass of NaCl [g/mol]
+    P_p = 14.696;         % permeate pressure, atmospheric for now [psi]
+    R = 8.3145;           % universal gas constant [J/(mol*K)]
+    MW_salt_avg = 30.548; % average molar mass of NaCl [g/mol]


I might be misunderstanding the definitions of M_salt and MW_salt_avg, but why are these not the same? Maybe you just need a clearer description in the comment

rebeccamccabe · 2022-11-02T02:26:17Z

Haefner_IPHROS_MOGA.m

+
+% Function for calculating R (symbolic)
+function R = R_value(Q_p)
+    R = 1.0034 - (12.2124*(Q_p^(-0.8122))); % -


Again constants don't match eq 37, probably a units issue

rebeccamccabe · 2022-11-02T02:27:11Z

Haefner_IPHROS_MOGA.m

+
+% Function for calculating Aw (symbolic)
+function Aw = Aw_value(Q_p,c_p)
+    term1 = -85.71804787;


this number should be -24.77 according to equation 41

rebeccamccabe · 2022-11-02T02:28:00Z

Haefner_IPHROS_MOGA.m

+function Aw = Aw_value(Q_p,c_p)
+    term1 = -85.71804787;
+    term2 = 3.3699665311*log(c_p);
+    term3 = 53.163612047*log(Q_p);


this number should be -.89 according to eq41

rebeccamccabe · 2022-11-02T02:28:34Z

Haefner_IPHROS_MOGA.m

+    term4 = 3.2938449078*log(c_p)*log(c_p);
+    term5 = 0.726912142*log(c_p)*log(c_p)*log(c_p);
+    term6 = 0.0516395755*log(c_p)*log(c_p)*log(c_p)*log(c_p);
+    term7 = -12.22459358*log(Q_p)*log(Q_p);


should be 0.27

rebeccamccabe · 2022-11-02T02:28:57Z

Haefner_IPHROS_MOGA.m

+    term5 = 0.726912142*log(c_p)*log(c_p)*log(c_p);
+    term6 = 0.0516395755*log(c_p)*log(c_p)*log(c_p)*log(c_p);
+    term7 = -12.22459358*log(Q_p)*log(Q_p);
+    term8 = 1.1774872012*log(Q_p)*log(Q_p)*log(Q_p);


should be -.26

matt-h-17 added 6 commits November 1, 2022 00:04

Add files via upload

6286e75

Supplemental material for "Integrated Pumped Hydro Reverse Osmosis System Optimization Featuring Surrogate Model Development in Reverse Osmosis Modeling"

Create ReadMe

57fad34

Update ReadMe

181b644

Update ReadMe

c585774

Update ReadMe

c99e553

Changed 3 variable names. No other changes made.

fc1e1ec

rebeccamccabe commented Nov 1, 2022

View reviewed changes

rebeccamccabe commented Nov 2, 2022

View reviewed changes

Scrutineering review #2

Are you sure you want to change the base?

Scrutineering review #2

Uh oh!

Conversation

rebeccamccabe commented Nov 1, 2022

Uh oh!

rebeccamccabe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants