Skip to content
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

bugfix: stdstars and relsymlink #1891

Merged
merged 2 commits into from Oct 31, 2022
Merged

bugfix: stdstars and relsymlink #1891

merged 2 commits into from Oct 31, 2022

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Oct 31, 2022

This PR fixes #1890, i.e. a bug introduced with relative symlinks in PR #1888 that broke the symlinks creation for multi-exposure stdstar fitting. This PR adds two redundant fixes:

  1. relsymlink(src, dst) now accepts relative paths for src instead of requiring two absolute paths and deriving the relative path. If src is a relative path, it is relative to dirname(dst), not relative to the current directory (same behavior as os.path.symlink). I added tests to confirm this behavior.
  2. desi_proc_joint_fit no longer derives the relative path, leaving it to relsymlink instead. In this case desi_proc_joint_fit was deriving the correct path anyway so fix (1) would have been sufficient, but it has the possible future fragility of /dvs_ro/cfs vs. /global/cfs, so I also updated it to let relsymlink standardize how relative paths are computed, since os.path.relpath isn't sufficient for all cases due to read-only vs. read-write mounts of the same directories.

Extra: I also updated some log.debug(...) messages to use log.debug("format string %s", blat) instead of log.debug(f"format string {blat}") so that it doesn't spend time evaluating the string if it isn't going to log it. That has mattered in the past where debug statements were in loops and ended up taking a significant fraction of the compute time. In this case it probably doesn't matter, but since I was updating a log.debug message anyway for the link variable name, I also updated the formatting method and then did the same for several others nearby.

Test run in /global/cfs/cdirs/desi/users/sjbailey/spectro/redux/p8r night 20201214, in particular tile 80605 exposures 00067710, 00067711, 00067712, 00067713.

@sbailey sbailey added this to In progress in Himalayas via automation Oct 31, 2022
@akremin
Copy link
Member

akremin commented Oct 31, 2022

The changes look good thanks. There was a failure in cholesky_solve, unrelated to this PR. I am rerunning the test now then I can merge.

@akremin akremin merged commit 0339b9d into main Oct 31, 2022
Himalayas automation moved this from In progress to Done Oct 31, 2022
@akremin akremin deleted the relsymlink branch October 31, 2022 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

relsymlink broke multi-exp stdstar fit
2 participants