turn OFF the mild warning msg of time cross leap second table #106
Merged
yunjunz merged 3 commits intoinsarlab:mainfrom Jan 21, 2026
Merged
turn OFF the mild warning msg of time cross leap second table #106yunjunz merged 3 commits intoinsarlab:mainfrom
yunjunz merged 3 commits intoinsarlab:mainfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideDisables leap-second boundary warning messages in the Fortran solid Earth tide code, modernizes the CI Miniforge installer to be platform-dynamic, and adds simple success prints to two test modules. Sequence diagram for updated dynamic Miniforge installer in CIsequenceDiagram
participant CI as CircleCI_job_build
participant EX as Linux_executor
participant SH as Shell
participant WG as wget
participant MF as Miniforge3.sh
participant ME as Miniforge_env
CI->>EX: start job
EX->>SH: run setup script
SH->>WG: download Miniforge3-$(uname)-$(uname -m)
WG-->>SH: Miniforge3.sh saved
SH->>MF: bash Miniforge3.sh -b -p ${HOME}/tools/miniforge
MF-->>ME: create Miniforge installation
SH->>ME: source conda.sh
SH->>ME: source mamba.sh
ME-->>SH: define CONDA_PREFIX and PATH
SH-->>CI: environment ready for subsequent steps
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new Miniforge wget command appears truncated (
...Miniforge3-$(uname)-$(uname -m).s), which will fail to download the installer; make sure the full.shfilename is used. - By hardcoding
if(.false.)insolid.foryou completely disable the leap-second boundary warning; consider keeping this behavior behind a flag or configuration option so users who rely on the warning can still enable it. - The
print('Pass.')additions in the tests add unnecessary noise to test output; it would be cleaner to rely on assertions only and drop these prints.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new Miniforge wget command appears truncated (`...Miniforge3-$(uname)-$(uname -m).s`), which will fail to download the installer; make sure the full `.sh` filename is used.
- By hardcoding `if(.false.)` in `solid.for` you completely disable the leap-second boundary warning; consider keeping this behavior behind a flag or configuration option so users who rely on the warning can still enable it.
- The `print('Pass.')` additions in the tests add unnecessary noise to test output; it would be cleaner to rely on assertions only and drop these prints.
## Individual Comments
### Comment 1
<location> `.circleci/config.yml:49` </location>
<code_context>
+ source "${HOME}/tools/miniforge/etc/profile.d/conda.sh"
+ source "${HOME}/tools/miniforge/etc/profile.d/mamba.sh"
# modify/export env var PATH to BASH_ENV to be shared across run steps
echo 'export PATH=${CONDA_PREFIX}/bin:${PATH}' >> ${BASH_ENV}
</code_context>
<issue_to_address>
**issue (bug_risk):** Relying on CONDA_PREFIX here may result in an empty or incorrect PATH if no environment is activated.
Because `CONDA_PREFIX` is only set after `conda activate`, and there’s no activation here, it may be unset and yield a bad PATH (e.g. `PATH=/bin:$PATH`). If you just need `miniforge/bin`, consider exporting a fixed install path like `${HOME}/tools/miniforge/bin` instead of relying on `CONDA_PREFIX`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
+ `.circleci/config.yml`: replace `mamba init bash` with `source conda/mamba.sh`, as suggested by its GitHub repo readme at https://github.com/conda-forge/miniforge?tab=readme-ov-file#as-part-of-a-ci-pipeline, to fix the current circle CI environment setup error.
because 1) no leap second has been introduced since Jan 2017 (https://data.iana.org/time-zones/tzdb/leap-seconds.list), and 2) the error introduced for missing a few leap second is small, and the accuracy would still be enough for many applications (https://geodesyworld.github.io/SOFTS/solid.htm) If there is new leap second announced by IERS, we will update the code accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR turns OFF the mild warning msg of time cross leap second table hardcored in
solid.forbecause 1) no leap second has been introduced since Jan 2017 (https://data.iana.org/time-zones/tzdb/leap-seconds.list), and 2) the error introduced for missing a few leap second is small, and the accuracy would still be enough for many applications (https://geodesyworld.github.io/SOFTS/solid.htm)If there is new leap second announced by IERS, we will update the code accordingly.
Summary by Sourcery
Disable leap-second boundary warning messages in the solid Earth tide Fortran routine and update CI Miniforge installation along with minor test output tweaks.
Bug Fixes:
Enhancements:
CI: