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

tile QA defaults / usability #1430

Merged
merged 2 commits into from Oct 7, 2021
Merged

tile QA defaults / usability #1430

merged 2 commits into from Oct 7, 2021

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Oct 3, 2021

This PR adds some convenience features to simplify the daily tile VI procedure:

  • updated desi_tile_vi defaults so that you can just run "desi_tile_vi" from a surveyops/ops svn directory without having to specify extra arguments:
    • --infile defaults to tiles-specstatus.ecsv instead of tiles.csv (which was both wrong and now crashes anyway since QA columns have been removed from tiles.csv)
    • --user defaults to unix username
  • desi_update_tiles_specstatus
    • checks if input tiles-specstatus.ecsv is up-to-date or needs an svn update first; use --force to override (at your own peril... svn merge conflicts are likely if you aren't up-to-date first)
    • --specstatus now defaults to "tiles-specstatus.ecsv"
    • new --dry-run option to calculate what would be updated without saving any files

Standard usage would now be:

export SPECPROD=daily
cd (directory where you have surveyops/ops checkout)
svn update .
desi_update_tiles_specstatus --dry-run
desi_update_tiles_specstatus
desi_tile_vi
svn commit -m "tile QA for YEARMMDD"

The desi_update_tiles_specstatus --dry-run is optional, but a useful cross check before changing anything, and both desi_update_tiles_spectstatus commands will complain if you forgot to svn update first.

@schlafly @araichoor @julienguy

@coveralls
Copy link

coveralls commented Oct 3, 2021

Coverage Status

Coverage decreased (-0.005%) to 26.623% when pulling 221b85a on specstatus into 9c654e6 on master.

@schlafly
Copy link
Contributor

schlafly commented Oct 5, 2021

I promised I'd suggest some additional usability improvements:

  • support for people with long names (already addressed)
  • "only new exposure" mode in desi_tile_vi. I think we want people doing daily maintenance of newly observed exposures to only see the new exposures, and not old "unsure" observations; if someone's said they're unsure, we want an expert to look at it, not for these likely problematic tiles to be mixed in with a bunch of likely good tiles until a non-expert just validates them and whatever issue was present gets lost.
  • I think desi_tile_vi should print a summary at the end that would be appropriate for copy/pasting to #survey-ops. e.g.
type valid unsure invalid
new dark
new bright
new other
old dark
old bright
old other
very old dark
very old bright
very old other

Here's I'm imagining separating the new ones (defined as "formerly had 'none' as QA"), the old ones (not none, and lastnight within 2 weeks?), and the very old ones (not none, and lastnight older than 2 weeks?).

@julienguy
Copy link
Contributor

desi_tile_vi already has the option --new which I think does exactly what you want.
I'm adding the option in the command line on the wiki page.

@schlafly
Copy link
Contributor

schlafly commented Oct 5, 2021

Thanks, that looks exactly right, sorry I missed it.

@sbailey
Copy link
Contributor Author

sbailey commented Oct 6, 2021

@schlafly I like the idea of the summary table and have also wanted that when running desi_tile_vi. Could you take a pass at adding that? If it is at all tricky and/or if you don't have the time to do it ~today, I suggest that we accept 2/3 bonus features as good enough and merge this and then spawn that request into a separate ticket so that it doesn't block the other features of this PR.

@schlafly
Copy link
Contributor

schlafly commented Oct 7, 2021

I can take a pass at this but it won't be today. I don't think we need to conserve merges and am happy if you want to merge now.

@schlafly
Copy link
Contributor

schlafly commented Oct 7, 2021

I added my features:

  • save every time
  • remove save / exit options (can just Ctrl-D or Ctrl-C to exit, since it saves every time)
  • pass is renamed "unsure"
  • print a summary table at end
    I was able to use this for my VI run earlier today, but I can't say that I've tested it more than that.

@sbailey
Copy link
Contributor Author

sbailey commented Oct 7, 2021

Live use for real QA is good enough testing. Merging now. Thanks for adding the additional features.

@sbailey sbailey merged commit 11f9d3c into master Oct 7, 2021
@sbailey sbailey deleted the specstatus branch October 7, 2021 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants