Maintenance in porepy.ad

[vlipovac]

Proposed changes

1. Introduction of intermediate Operator base classes handling arbitrary depth of previous iteration and time step.
2. Re-work in forward evaluation: Removing double recursion by introducing parse for variables and replacing projection of global forward AdArray by slicing (less expensive than matrix multiplication)
3. EquationSystem works internally now solely on Variable ID: Changes in some private data maps and fixing of global DOF order.
4. Maintenance on Operator functions: Proper type setting and abstraction compatible with multiple inheritance including intermediate base classes from Change No 1.
5. Adding tests to cover changes in prev iter and time step, as well as some basic tests of the operator function class.

Since the changes are a bit extensive, I am giving a walk-through in the form of a self-review.

EDIT:

Following the 1st round of reviews, the index convention for previous time steps is now unified with the convention for previous iterates:

• 0 represents now the current value
• 1 represents the first previous value

Since current time (unknown value) is given by time_step_index = 0 and any iterate_index >= 0, it would be ambiguous to use the time step index. (and also unnecessary to store same data twice).
Therefore time_step_index = 0 is now not supported anymore.

Types of changes

• Minor change (e.g., dependency bumps, broken links).
• Bugfix (non-breaking change which fixes an issue).
• New feature (non-breaking change which adds functionality).
• Breaking change (fix or feature that would cause existing functionality to not work as expected).
• Testing (contribution related to testing of existing or new functionality).
• Documentation (contribution related to adding, improving, or fixing documentation).
• Maintenance (e.g., improve logic and performance, remove obsolete code).
• Other:

Checklist

• The documentation is up-to-date.
• Static typing is included in the update.
• This PR does not duplicate existing functionality.
• The update is covered by the test suite (including tests added in the PR).
• If new skipped tests have been introduced in this PR, pytest was run with the --run-skipped flag.

[IvarStefansson]

This looks extremely useful, thanks! I did a curiosity motivated review, deferring to @keileg or @OmarDuran for the proper review. Please bear with my stupid questions and somewhat random suggestions.

[keileg]

@vlipovac: So you know, I am working my way through the PR, but I'm not done yet. To be continued.

[keileg]

Nice work! Some remarks:

1. Thanks for taking care of the parsing of variables in the EquationSystem. The previous code will not be missed!
2. It seems there is a consensus to let the indexes of time steps and iterates both start at 0.
3. Overall, I think the cleanup in EquationSystem is to the good, although you are surely right this needs further clarifications, and quite likely refinement of the logic you have introduced.
4. My only major concern is the changes proposed to dofs_of, which (I believe) are contrary to the way I would expect such a method to behave. This is particularly relevant when working with solvers and similar. I suggest we hear from the solver guys before making any decisions there.

[IvarStefansson]

@keileg, regarding your comment

2. It seems there is a consensus to let the indexes of time steps and iterates both start at 0.
   I think it's wise to have a second look at all indexes and tags for iterates and time steps, including None and where we start counting (current time/iterate vs first previous time/iterate).

[vlipovac]

I pushed the easy/trivial suggestions and resolved respective conversations (including my self-review).
Open discussions are what's left.

[vlipovac]

@keileg @IvarStefansson
Regarding the discussion on unifying the time step indexation with the iterate indexation (time index 0 is current time):
This requires to introduce a new convention.
Since time index 0 is current time and we do not want to store same values twice, I propose making time_step_index=0 and iterate_index=0 equivalent throughout the framework.
(By "current time" we always refer to the current iterate, since otherwise it would ambiguous).
This convention must be implemented in pp.get_solution_values and pp.set_solutuon_values.
Any comments regarding that?

Also, this will introduce minor changes in several files, where time_step_index=0 was used for previous time,

[keileg]

2. I think it's wise to have a second look at all indexes and tags for iterates and time steps, including None and where we start counting (current time/iterate vs first previous time/iterate).

I'll see if I can have a look at some code later today, and will get back to this (soon, I hope).

[keileg]

@vlipovac: I have gone through the changes and resolved conversations with a somewhat heavy hand. In addition to what looks like minor fixes, there are two remaining issues (both discussed in some details in the relevant conversations):

1. Unifying the time_step_index and iterative_index in a way that leaves decent but not necessarily polished code (your time is better spent on the upcoming PRs)
2. Reverting the change (or at least functionality) in EquationSystem.dofs_of().

Let me know if you have comments or questions.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[keileg]

Thanks for the effort, @vlipovac