-
Notifications
You must be signed in to change notification settings - Fork 14
turn OFF the mild warning msg of time cross leap second table #106
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
EJFielding
left a comment
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.
Good solution, thanks!
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: