Open jeffeaton opened 1 month ago
I think there are a couple comments from my side:
foo
and then adding and subtracting foo
I agree that shorter variables are better and we should use them, there's no reason to write intermediate.base
everywhere, so the easiest way i can think of around this problem is:
so frogger code becomes
// technically these should be defined outside of the `ha` and `g` loops
// so just pretend they are outside any for loops
auto grad = intermediate.base.grad;
auto cd4_prog_prob = natural_history.cd4_progression;
auto hiv_adult = state_next.base.h_hiv_adult;
for (int hm = 1; hm < ss.hDS; ++hm) {
real_type n_adults_progressing = cd4_prog_prob(hm - 1, ha, g) * hiv_adult(hm - 1, ha, g);
grad(hm - 1, ha, g) -= n_adult_progressing;
grad(hm, ha, g) += n_adult_progressing;
}
Thanks @M-Kusumgar, yes this sort of declarations are exactly what I had in mind to make it more readable
// technically these should be defined outside of the `ha` and `g` loops
// so just pretend they are outside any for loops
auto grad = intermediate.base.grad;
auto cd4_prog_prob = natural_history.cd4_progression;
auto hiv_adult = state_next.base.h_hiv_adult;
for (int hm = 1; hm < ss.hDS; ++hm) {
real_type n_adults_progressing = cd4_prog_prob(hm - 1, ha, g) * hiv_adult(hm - 1, ha, g);
grad(hm - 1, ha, g) -= n_adult_progressing;
grad(hm, ha, g) += n_adult_progressing;
}
Or if we wanted to keep some indicative prefixing, we could do some assignment like:
auto nh = natural_history.cd4_progression;
...
for (int hm = 1; hm < ss.hDS; ++hm) {
real_type n_adults_progressing = nh.cd4_prog_prob(hm - 1, ha, g) * hiv_adult(hm - 1, ha, g);
grad(hm - 1, ha, g) -= n_adult_progressing;
grad(hm, ha, g) += n_adult_progressing;
}
yh im onboard with creating these shorter variables inside the smaller functions and the larger for loop for time step can still move around these structured parameters
Using prefixing at the top of functions sounds good, let's go through and add this to address this issue. Should we also add some structure to the paeds parameters, at the moment they are one flat list. @mwalte10 (see natural_history, demography etc within the base model parameters above)
I think we have discussed this before, but I can't recall details of the decision.
I find that the long class prefixes in the frogger code obscure the 'important bits' of the code (variable names and arithmetic operators) and consequently impede ability to quickly parse the code and figure out what a line of code is actually doing. It feels like this is a recipe for inviting errors if it is more difficult to figure out what code is actually doing.
E.g. this line of code in Leapfrog:
has become this:
In the first version, I can immediately see that "what is different" is
hm-1
in the left side of the first line andhm
in the second line -- and I can see from quick scan of the code that it's subtracting and adding the same thing. If there was a typo, I would be more likely to spot it.In the second frogger version, I have to really carefully read it to strip out all of the filler characters
intermediate.base.
,state_next.base.
,natural_history.
to figure out what the operative parts of the code are.At some point we discussed abbreviating the class names in the code at the top of the scripts, which I think could help this issue a lot.
@r-ash @mwalte10 @M-Kusumgar -- what do you think?