Time step and iteration indexation in pp.AD

[vlipovac]

Proposed changes

Fixing the convention regarding time and iterate indices.

1. Time step indexation reverted to start with 0 for first previous time step.
2. Time-dependent AD operators have None as their time step index, as long as they are at current time.
3. Iterative operators have always an integer value as iteration index, starting with 0 for the first previous iterate.
4. The first call to .previous_iteration() does not push the iterate index up (stays 0), but flags the instance as as previous iterate, removing it's capability to get a proper AdArray during parsing (.values_and_jacobian() will give a trivial .jac)

The "flag" for is previous iterate (also for time step) is computed using a PRIVATE index, which starts at -1, and is consistently increased with += 1.

Note also, I propose here to return iterate_index as None for operators which are both TimeDependentOperators and IterativeOperators, and at a previous time step.

1. The aim is to make it more clear to users, that there are no iteration indices for previous time steps.
2. Also, code in various parts of the framework will be slightly shorter, because the get_values and set_values methods do not need an if-branch regarding whether the variable/operator is at previous time. Variables will always have a pair of time step index and iterate index, which are consistent with the intended usage of various get and set values methods in the Ad framework.

Finally, note that I put the changes regarding shift_solution_values into this PR, since it fits better. They will be removed from PR #1170 once this goes through after updating that PR with develop.

Relevant changes

The essential changes are in the following files:

src/numerics/ad/_ad_utils.py src/numerics/ad/equation_system.py src/numerics/ad/operators.py

Changes in the other file revert essentially the changes introduced in PR #1166 (time_step_index = 0 not time_step_index=1)

[review-notebook-app[bot]]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[vlipovac]

@IvarStefansson @keileg @jwboth I had a discussion with Ivar and I would like to finish this PR as it is stalling progress in the chain of PRs regarding CF.

Essentially, I took the liberty to rename the stuff, having your comments in mind. The common denominator I was able to filter out was the wish to rename it to something resembling proper language and to add the word at. I strongly discourage the use of value anywhere, since per logic we have two representations of the mathematical model: One is more symbolic (Operators and Operator trees), the other numerical (parsing operators and getting arrays). I think we should be aware of these parallel structures, which of course go hand in hand, but their internal workings and functionality are orthogonal in some sense. This should be reflected in the naming convention. I.e., keywords like value should indicate a switch from the symbolic representation to the numeric evaluation. The keyword previous I kept to indicate the directional behavior of the functionality, backwards in time and the iterate sense.

So the members are renamed as follows:

Functions creating operators at previous X

1. previous_timestep() -> at_previous_time_step()
2. previous_iteration() -> at_previous_iteration()

Boolean flags checking the index

1. is_previous_timestep -> is_at_previous_time_step
2. is_previous_iterate -> is_at_previous_iteration
3. is_current_iterate -> is_at_current_iteration

The switch from timestep to time_step is made to be consistent with the time_step_index, which a property but also an argument in the equation system and ad utility functions.

@IvarStefansson If you agree with this, please resolve the respective conversation and let's finalize this, but vennligst have a final thought on the changes here so we don't rush into pushing something confusing like in the previous PR.

[jwboth]

@IvarStefansson @keileg @jwboth I had a discussion with Ivar and I would like to finish this PR as it is stalling progress in the chain of PRs regarding CF.

Essentially, I took the liberty to rename the stuff, having your comments in mind. The common denominator I was able to filter out was the wish to rename it to something resembling proper language and to add the word at. I strongly discourage the use of value anywhere, since per logic we have two representations of the mathematical model: One is more symbolic (Operators and Operator trees), the other numerical (parsing operators and getting arrays). I think we should be aware of these parallel structures, which of course go hand in hand, but their internal workings and functionality are orthogonal in some sense. This should be reflected in the naming convention. I.e., keywords like value should indicate a switch from the symbolic representation to the numeric evaluation. The keyword previous I kept to indicate the directional behavior of the functionality, backwards in time and the iterate sense.

So the members are renamed as follows:

Functions creating operators at previous X

1. previous_timestep() -> at_previous_time_step()
2. previous_iteration() -> at_previous_iteration()

Boolean flags checking the index

1. is_previous_timestep -> is_at_previous_time_step
2. is_previous_iterate -> is_at_previous_iteration
3. is_current_iterate -> is_at_current_iteration

The switch from timestep to time_step is made to be consistent with the time_step_index, which a property but also an argument in the equation system and ad utility functions.

@IvarStefansson If you agree with this, please resolve the respective conversation and let's finalize this, but vennligst have a final thought on the changes here so we don't rush into pushing something confusing like in the previous PR.

I give all authority to @IvarStefansson - I will accept whatever will be decided here.

But since the final decision is not made yet, and before it is, here is one more possibility for your inspiration, motivated by my aversion to too long names: at_time_step(lag=i) and acordingly for iteration, where i=0 gives the current and i=1 gives the previous one. This allows to unify with current time steps/iterates as well, e.g., is_at_iteration(lag=i) removes the need for splitting up the boolean methods (I could not find those in the code - maybe I misunderstood and these are not methods...)

Maybe there is a better word than lag. Yet, I wanted to consisely indicate the backward nature of what we use to call index, which is counterintuitive considering how one would write time discretizations and iterative methods on paper. In general, now having looked at the code, and with the above. Such a change would strongly suggest also renaming time_step_index to time_step_lag.

To make it even more complicated, one could provide two possibilities to access the operators and also get_solution_values, using not only indices of type int, but a Literal["current", "previous"], e. g.,at_iterate("current"),at_iterate("previous"),get_solution_values(time_step="current"` etc. Of course a nightmare to maintain. But as your main motivation is expressivity of the code, my answer would look like that.

I anticipate that this is not of interest. Nevertheless, since I was blank last time I was asked and these thoughts now popped into my head, here it is: my brain dump :)

[keileg]

@vlipovac @IvarStefansson, @jwboth: It seems to me that, while we agree the current previous_timestep, previous_iterate should be renamed, we are not ready to conclude on a naming scheme. Still, Veljko is right we should finalize this process so as not to block the merges for compositional flow. I therefore suggest we revert the naming to what it was before this PR and revisit at a later stage. From what I understand, such a resolution will allow us to merge this PR with little extra effort.

Does this make sense to you?

[IvarStefansson]

@vlipovac @IvarStefansson, @jwboth: It seems to me that, while we agree the current previous_timestep, previous_iterate should be renamed, we are not ready to conclude on a naming scheme. Still, Veljko is right we should finalize this process so as not to block the merges for compositional flow. I therefore suggest we revert the naming to what it was before this PR and revisit at a later stage. From what I understand, such a resolution will allow us to merge this PR with little extra effort.

Does this make sense to you?

Yes, I think this is a good solution!

[vlipovac]

@vlipovac @IvarStefansson, @jwboth: It seems to me that, while we agree the current previous_timestep, previous_iterate should be renamed, we are not ready to conclude on a naming scheme. Still, Veljko is right we should finalize this process so as not to block the merges for compositional flow. I therefore suggest we revert the naming to what it was before this PR and revisit at a later stage. From what I understand, such a resolution will allow us to merge this PR with little extra effort. Does this make sense to you?

Yes, I think this is a good solution!

It's a bit of a pity that we failed to include this trivial change here, are you sure about this? To my understanding, the target revision to return to would be 6016bf2, correct?

Let me just clarify on thing regarding the booleans: They are decorated with @property, hence not callable. The value is computed based on the index value, and it's True for all instances at some index, other than the current one. Implementing a callable check by passing an index, makes to me little sense, primarily because this is a duplicate functionality (the user can simply check the value of the index, which is a public property of these operators)

[keileg]

It's a bit of a pity that we failed to include this trivial change here, are you sure about this?

I agree it would be better to make a change we are all happy with now and be done with it. Problem is, we have not yet managed to decide precisely what we want to do, despite a week of discussions. My experience in such situations is, we can go on until we reach a point of exhaustion and make a decision that has a high risk of being suboptimal. The better option usually is to cut the losses for now, and revisit the topic at a later point, so that is what I suggest we do.

To my understanding, the target revision to return to would be 6016bf2, correct?

Yes.

[vlipovac]

I agree it would be better to make a change we are all happy with now and be done with it. Problem is, we have not yet managed to decide precisely what we want to do, despite a week of discussions. My experience in such situations is, we can go on until we reach a point of exhaustion and make a decision that has a high risk of being suboptimal. The better option usually is to cut the losses for now, and revisit the topic at a later point, so that is what I suggest we do.

Reverted the changes two commits back.

For future reference

Reverted commits with changes in naming:

051e645 7cc6c22