Feature/dom eig #710
Feature/dom eig #710
Conversation
|
I have addressed most of the comments and ready for the second round of reviews. |
| SUNDomEigEst_ARNOLDI Usage | ||
| ------------------------------- | ||
|
|
||
| The header file to be included when using this module is ``sundomeigest/sundomeigest_arnoldi.h``. |
There was a problem hiding this comment.
@gardner48, is this still a true statement after your adjustments in cmakes?
| SUNDomEigEst_Power Usage | ||
| ----------------------------- | ||
|
|
||
| The header file to be included when using this module is ``sundomeigest/sundomeigest_power.h``. |
There was a problem hiding this comment.
@gardner48, is this still a true statement after your adjustments in cmakes?
| Note that although a DEE creation routine requires :c:func:`SUNDomEigEst_SetATimes` with a valid | ||
| matrix-vector product function pointer, creating a DEE with :c:func:`LSRKStepSetDomEigFn` | ||
| uses an internal Jacobian-vector product estimation that is passed with the *arkode_mem* pointer. | ||
| Similarly, it estimates the eigenvalue as needed internally without requiring user to call | ||
| :c:func:`SUNDomEigEstimate` function. | ||
| Similarly, it estimates the eigenvalue as needed internally without requiring a call to | ||
| :c:func:`SUNDomEig_Estimate`. |
There was a problem hiding this comment.
I don't think that I understand this paragraph. Is it saying that if a user provides a custom ARKDomEigFn (through calling LSRKStepSetDomEigFn) then the matrix-vector product is approximated internally by LSRKStep? I don't think that is correct.
Should this (and the previous paragraph) instead be something like:
":c:func:LSRKStepSetDomEigFn expects a user-provided spectral radius function pointer of type :c:func:ARKDomEigFn. If this is called with dom_eig set to NULL, then LSRKStep will construct a DEE for this purpose, that will internally approximate the Jacobian-vector product for the DEE using the supplied right-hand side function."
| has already been created. TODO: Add extra information about how we implement this function | ||
| after deciding on it by the group. |
There was a problem hiding this comment.
It's unclear to me what is needed for this "TODO". Generally, we describe the "mathematics" of the algorithms elsewhere in the documentation, and refer back to that here. If the question refers to what should be described about the algorithms, then my response is that inside the relevant file, all aspects of the algorithm should either be cited from publications or should be described in sufficient detail so that the inputs to each "set" routine make sense.
Given that you implemented these as standalone SUNDIALS utilities, the proper location to describe those algorithms is in doc/sundomeigest/SUNDomEigEst_Arnoldi.rst and doc/sundomeigest/SUNDomEigEst_Power.rst. I would then add a reference to those here similar to "Information on the SUNDomEigEstimator implementations provided by SUNDIALS may be found in :ref:SUNDomEigEst.Introduction."
| **Arguments:** | ||
| * *arkode_mem* -- pointer to the LSRKStep memory block. | ||
| * *DEE_id* -- ID of the dominant eigenvalue estimator (of type c:enum::`SUNDomEigEstimator_ID`). | ||
| * *DEE* -- pointer to the SUNDIALS Dominant Eigenvalue Estimator (of type :c:type:`SUNDomEigEstimator`). |
There was a problem hiding this comment.
This is the SUNDomEigEstimator itself, not a pointer:
| * *DEE* -- pointer to the SUNDIALS Dominant Eigenvalue Estimator (of type :c:type:`SUNDomEigEstimator`). | |
| * *DEE* -- the SUNDIALS Dominant Eigenvalue Estimator to use (of type :c:type:`SUNDomEigEstimator`). |
| Although, a DEE creation routine requires :c:func:`SUNDomEigEstSetATimes` with a valid | ||
| matrix-vector product function pointer, creating a DEE with :c:func:`LSRKStepDomEigEstCreate` | ||
| Note that although a DEE creation routine requires :c:func:`SUNDomEigEst_SetATimes` with a valid | ||
| matrix-vector product function pointer, setting a DEE with :c:func:`LSRKStepSetDomEigEstimator` | ||
| uses an internal Jacobian-vector product estimation that is passed with the *arkode_mem* pointer. | ||
| Similarly, it estimates the eigenvalue as needed internally without requiring user to call | ||
| :c:func:`SUNDomEigEstimate` function. | ||
| Similarly, it estimates the eigenvalue as needed internally without requiring a call to | ||
| :c:func:`SUNDomEig_Estimate`. |
There was a problem hiding this comment.
I recommend that this paragraph be removed entirely, since it's essentially identical to what is included for the previous function.
| .. note:: If LSRKStepSetNumSucceedingWarmups routine is not called, then the default ``num_succ_warmups`` is set to :math:`0`. | ||
| Calling this function with ``num_succ_warmups < 0`` resets the default. |
There was a problem hiding this comment.
I'd indent this so that the "Note" is included as part of this function's documentation:
| .. note:: If LSRKStepSetNumSucceedingWarmups routine is not called, then the default ``num_succ_warmups`` is set to :math:`0`. | |
| Calling this function with ``num_succ_warmups < 0`` resets the default. | |
| .. note:: If LSRKStepSetNumSucceedingWarmups routine is not called, then the default ``num_succ_warmups`` is set to :math:`0`. | |
| Calling this function with ``num_succ_warmups < 0`` resets the default. |
| A ``num_warmups`` argument that is :math:` < 0` will result in the default | ||
| value (100). This default value is particularly chosen to minimize the memory | ||
| footprint by lowering the required ``kry_dim``. |
There was a problem hiding this comment.
Please add a comment regarding whether these warmups are performed before every "Estimate" call, or only once.
| ``SUNDomEigEstimator``. | ||
|
|
||
| **Arguments:** | ||
| * *q* -- a template vector. |
There was a problem hiding this comment.
Is q really just a template vector, or it it actually very first initial guess at the dominant eigenvector? If it's the initial guess, then that should be explained here, as well as a warning that users should ensure that this is not exactly a non-dominant eigenvector of the Jacobian (e.g., they can use a random vector).
| There are ``SUNDomEigEstimator`` examples that may be installed for each | ||
| implementation; these make use of the functions in ``test_sundomeigest.c``. | ||
| These example functions show simple usage of the ``SUNDomEigEstimator`` family |
There was a problem hiding this comment.
These are unit tests, not examples (they are in /test/unit_tests/sundomeigest instead of /examples).
We typically do not document unit tests, but if you think that this documentation page would be particularly helpful for users, then we should discuss the optimal placement of this page in the documentation (and correspondingly, the location of the examples/tests it describes).
| Preprocessing warmups for power iteration refer to running power iteration without | ||
| checking for convergence. This can help reduce some computational overhead. | ||
| A ``num_warmups`` argument that is :math:` < 0` will result in the default | ||
| value (0). This default is chosen to minimize complexity for the general user. |
There was a problem hiding this comment.
Again, please comment on whether these warmup iterations occur before every estimate, or just once before the first estimate.
| * ``max_num_iters`` - maximum number of power iterations so far, | ||
|
|
||
| * ``min_num_iters`` - minimum number of power iterations so far, |
There was a problem hiding this comment.
| * ``max_num_iters`` - maximum number of power iterations so far, | |
| * ``min_num_iters`` - minimum number of power iterations so far, | |
| * ``max_num_iters`` - maximum number of power iterations in any single estimate so far, | |
| * ``min_num_iters`` - minimum number of power iterations in any single estimate so far, |
Add dominant eigenvalue estimator class --------- Co-authored-by: Daniel R. Reynolds <reynolds@smu.edu> Co-authored-by: Daniel R. Reynolds <dreynolds@umbc.edu> Co-authored-by: Balos, Cody, J <balos1@llnl.gov> Co-authored-by: David J. Gardner <gardner48@llnl.gov>
Add dominant eigenvalue estimator class --------- Co-authored-by: Daniel R. Reynolds <reynolds@smu.edu> Co-authored-by: Daniel R. Reynolds <dreynolds@umbc.edu> Co-authored-by: Balos, Cody, J <balos1@llnl.gov> Co-authored-by: David J. Gardner <gardner48@llnl.gov>
The pull request for the dominant eigenvalue estimator module.