-
Notifications
You must be signed in to change notification settings - Fork 19
Issue 1438 - Fixed parameter naming #1546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR standardizes parameter naming conventions in the wavefield writer configuration by converting from word_word_word (underscore) format to word-word-word (hyphen) format, ensuring consistency across the codebase.
- Updated parameter names in the wavefield writer from underscore to hyphen format
- Updated error messages to reflect the new parameter naming convention
- Added partial documentation for the renamed parameters
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/parameter_parser/writer/wavefield.cpp | Renamed four parameters (time-interval, time-interval-by-memory, include-last-step, for-adjoint-simulations) from underscore to hyphen format; updated error message; removed trailing whitespace |
| docs/sections/parameter_documentation/index.rst | Added documentation for time-interval and include-last-step parameters; added example usage in YAML block |
| docs/sections/api/index.rst | Reordered API documentation list (moved specfem/macros before specfem/mpi for alphabetical ordering) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| Time step interval for writing the wavefield. | ||
|
|
||
| :default value: 1 (-> every time step) |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documented default value for time-interval is "1 (-> every time step)" but the code returns 0 when the parameter is not provided. This inconsistency between documentation and implementation should be corrected. Either update the default value in the code to return 1, or update the documentation to reflect that the actual default is 0.
| :default value: 1 (-> every time step) | |
| :default value: 0 |
| :default value: False | ||
| :default value: True | ||
|
|
||
| :possible values: [bool] |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter for-adjoint-simulations (renamed from for_adjoint_simulations) is missing from the documentation. This parameter is used in the code but has no corresponding documentation entry explaining its purpose, default value, and possible values. Documentation should be added for this parameter in the wavefield section.
| :default value: False | ||
| :default value: True | ||
|
|
||
| :possible values: [bool] |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter time-interval-by-memory (renamed from time_interval_by_memory) is missing from the documentation. This parameter is used in the code but has no corresponding documentation entry explaining its purpose, default value, and possible values. Documentation should be added for this parameter in the wavefield section.
Description
writer parameter names all follow
word-word-wordconvention instead of some this way and some inword_word_wordfashion.Also edited the parameter description as well.
Commits
Issue Number
Closes #1438
Checklist
Please make sure to check developer documentation on specfem docs.