Skip to content

Point source merge#866

Open
rwalkerlewis wants to merge 65 commits intogeodynamics:mainfrom
rwalkerlewis:point_source_merge
Open

Point source merge#866
rwalkerlewis wants to merge 65 commits intogeodynamics:mainfrom
rwalkerlewis:point_source_merge

Conversation

@rwalkerlewis
Copy link
Copy Markdown
Contributor

Adds moment tensor point sources. Includes source class and documentation describing class and components.

rwalkerlewis and others added 30 commits December 31, 2025 05:00
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to this file are not relevant. Please remove.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gmsh files should be the default for all examples. Use Python scripts to generate Cubit meshes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename directory to something more descriptive. Is the point source for a dynamic simulation or quasistatic? Can it fit within an existing example as an additional step?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference between step01 and step02 should be clear by the filename.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyLith coding style includes comments for all closing braces. Please revert this change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this line being commented out? This PR is related to point sources, not poroelasticity.

Copy link
Copy Markdown
Contributor

@baagaard-usgs baagaard-usgs Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a step function? If so, rename to StepFunction.

Remove unused auxiliary fields (center frequency) and debugging code that is commented out.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change does not appear to be related to point sources.

Copy link
Copy Markdown
Contributor

@baagaard-usgs baagaard-usgs Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this called ...MomentTensorForce and not ...MomentTensor?

There is a lot of duplicate code here among the AuxiliaryFactor classes. Refactor to remove duplicate code.

err = VecCreateSeqWithArray(PETSC_COMM_SELF, dim, _pointCoords.size(),
&_pointCoords[0], &vecPoints);PYLITH_CHECK_ERROR(err);

// Debug
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove debugging code

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes do not appear to be related to point sources.

Copy link
Copy Markdown
Contributor

@baagaard-usgs baagaard-usgs Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is user-specific and should not be checked into the repository.

Copy link
Copy Markdown
Contributor

@baagaard-usgs baagaard-usgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High-level comments:

  1. The commit history is extremely messy. There should be no rebase commits. The workflow is not clear to other developers. Separate changes into several simple sets of commits (changes to libsrc, implementation of libtests, implementation of MMS tests, changes to modules, changes to Python, changes to examples, changes to docs).
  2. Remove changes not related to point sources (these may all be coming from rebasing, which should never introduce commits in your branch).
  3. The directory name used in the example is inconsistent with the other examples. Can these examples fit within the context of any existing example? For example, put a point source in the middle of strikeslip-2d to show a similar deformation field to the corresponding Green's function.
  4. All examples must use Gmsh files as the default. Cubit meshes can be provided as alternatives (see existing examples). Convert any Cubit journal scripts to Python scripts for consistency with the rest of the repo.
  5. The C++ implementations of the point source time functions (including kernels) should be in their own directory. Given them appropriate names, like those for the kinematic source time functions used in FaultCohesiveKin. Why are the sources all called wavelets? The step function, Gaussian, and time history do not seem to be associated with wavelets. Do you really need separate kernels for 2D and 3D?
  6. Remove debugging code that is commented out, especially if it is PetscPrintf() (journal debugging code can be acceptable if it is likely to be used in debugging MMS or full-scale tests).

Recommended path forward:

  1. Create an issue with the proposed layout of the files, including source implementation, tests, examples, and documentation. List names of classes and files. This will permit converging on the naming scheme and code layout.
  2. I would create a new branch and a new, clean commit history rather than trying to clean up the existing one.

Note: Additional comments are available on some files. I didn't closely examine the implementation details because the commit history is messy and the PR includes changes unrelated to point sources. Once a PR contains a clean commit history with appropriate code layout and names, it will be easier to review and suggest incremental changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants