Closed mcb64 closed 1 year ago
:+1: on beta release I'll take a stab at reviewing
Looks like camViewer
from engineering tools uses run_viewer.csh
(note: csh not sh):
There's also an annoying thing when sourcing conda environment activation scripts in bash scripts:
(base) [klauer@psbuild-rhel7-02 R3.0.0-rc1]$ ./run_viewer.sh --help
usage: source ./run_viewer.sh
Source this to activate a pcds conda environment.
By default, this activates the latest environment.
Use export PCDS_CONDA_VER=<version> before running to pick a different env.
I've found that doing this works, to ensure that our command-line arguments don't get passed in:
source /cds/group/pcds/pyps/conda/pcds_conda ""
(aside: conda run -p ...
is the ideal invocation here, but it's still a bit broken in my testing)
Once I got past the above, I was able to view a camera successfully and had the new projection plots
Hmm. Not sure what that "force push" was... I was trying to push here, not to the main line. Also not sure why I can't comment on Ken's last comment... but I've added the "" and a link for .csh.
Annoyingly, the comments we're making right now in the PR itself aren't threaded in any way - so they're just inlined with the commit messages you're making. It's distinct from the "comment on a line on a file in the PR" comment system, for some odd reason.
Not sure about the force push, but apparently you at least changed a commit message or something (commit ordering? source code? number of commits? 🤷 ) before force pushing.
Generally we avoid force pushing in PRs because it muddies the reviewing waters even more. It's not as bad as the forbidden "push to master of the repository", though.
Yeah, I avoid force pushing too. Wanted to push to mcb64, and didn't use --force, so I'm not sure what happened there...
@ZLLentz I think probably not (?), based on the ones that are set to latest:
$ ll */camviewer |grep latest
lrwxrwxrwx 1 bhill ps-pcds 38 Feb 13 2015 amo/camviewer -> /reg/g/pcds/pyps/apps/camviewer/latest
lrwxrwxrwx 1 bhill ps-pcds 43 Nov 10 2020 det/camviewer -> /cds/group/pcds/pyps/apps/camviewer/latest/
lrwxrwxrwx 1 dflath ps-pcds 38 Oct 14 2014 fee/camviewer -> /reg/g/pcds/pyps/apps/camviewer/latest
lrwxrwxrwx 1 bhill ps-pcds 38 Aug 22 2017 hpl/camviewer -> /reg/g/pcds/pyps/apps/camviewer/latest
lrwxrwxrwx 1 dflath ps-pcds 38 Oct 15 2014 las/camviewer -> /reg/g/pcds/pyps/apps/camviewer/latest
lrwxrwxrwx 1 bhill ps-pcds 38 Mar 27 2018 mcc/camviewer -> /reg/g/pcds/pyps/apps/camviewer/latest
lrwxrwxrwx 1 sstubbs ps-pcds 38 Jun 13 2016 sxr/camviewer -> /reg/g/pcds/pyps/apps/camviewer/latest
lrwxrwxrwx 1 aegger ps-pcds 16 Sep 14 2020 tmo/camviewer -> camviewer_latest
lrwxrwxrwx 1 trendahl ps-pcds 38 Jun 5 2017 xrt/camviewer -> /reg/g/pcds/pyps/apps/camviewer/latest
Edit: oh, latest isn't symlinked as Mike indicates below. None are pointing to the alpha either.
Doesn't look like anyone has switched. I noticed some issue with color cameras, and have pushed a fix for that into this. I'm also not liking how the projections are not the ROI width, but the layout math is eluding me at the moment, so probably won't fix that until tomorrow.
At some point, whether it's had any proper testing or not, once this has been merged in I'll release R3.0.0 and make it latest. Then we'll see... :-)
I've tagged the latest with fixes as R3.0.0-rc2. I've also checked this out into the -rc1 release area... I don't want to bother people who haven't even looked at this yet with a second directory name. I'll delete this after we have an actual release anyway.
So... any further ideas/suggestions here? Do we wait a bit longer to get some actual usage?
Now seems like a pretty reasonable time to merge/deploy/have people test to me, at least
I agree with Ken, it's a good time to roll with it
I've changed latest to point to my R3.0.0-rc1 directory, but this doesn't really change much: amo, det, hpl, las, mcc, sxr, tmo, and xrt seem to use latest, but among these only, det, las, and las would use the camviewer at all.
Sigh.
Maybe:
I think we've played with this long enough. Could one of you approve so we can merge? Then I'll blackify everything quickly for another pull request... and finally, the gaussian/supergaussian fits!
It's probably easier to list what hasn't changed here. But anyway, the changes include:
I think this needs eyes and a beta release. I'm going to do the beta from my fork so we can get started now, because patience has never been one of my virtues. But please take a look at this.