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

adds option for reduced stills_process logging #2263

Merged
merged 9 commits into from
Dec 15, 2022
Merged

adds option for reduced stills_process logging #2263

merged 9 commits into from
Dec 15, 2022

Conversation

dermen
Copy link
Contributor

@dermen dermen commented Oct 27, 2022

When running jobs in the terminal, this option suppresses a lot of refinement logging, and provides minimal progress feedback to user.

@dermen dermen requested a review from phyy-nx October 27, 2022 15:29
@phyy-nx
Copy link
Member

phyy-nx commented Oct 27, 2022

Awesome, this has been high on my list as well. Big carbon footprint reduction :)

I'll review soon, but I'm inclined to make this default, maybe with a couple adjustments to current logging calls to bubble up one or two key items.

@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #2263 (1610530) into main (c51410c) will increase coverage by 0.00%.
The diff coverage is 64.28%.

@@           Coverage Diff           @@
##             main    #2263   +/-   ##
=======================================
  Coverage   80.54%   80.54%           
=======================================
  Files         586      586           
  Lines       67011    67024   +13     
  Branches     8924     8926    +2     
=======================================
+ Hits        53972    53984   +12     
- Misses      10975    10976    +1     
  Partials     2064     2064           

@phyy-nx
Copy link
Member

phyy-nx commented Nov 8, 2022

@ndevenish I added handlers for xfel modules so that they could use the logging in dials. Is ok? See cctbx/cctbx_project#819. When that is merged, and with my new commits above, on this branch using @dermen's logging_option=disabled, dials.stills_process is utterly silent :)

@phyy-nx
Copy link
Member

phyy-nx commented Nov 8, 2022

File sizes for the logs from the 6 image SACLA h5 dataset used by test_stills_process:

  • Main: 377K
  • This branch, logging_option=suppressed (the new default): 75K
  • This branch, logging_option=normal: 372K (the SauterPoon outlier printouts are now hidden unless sauter_poon.verbose=True
  • This branch, logging_option=disabled: 0K

@phyy-nx
Copy link
Member

phyy-nx commented Nov 16, 2022

@ndevenish ok to merge? The xfel handler ok? Thanks and no hurry :)

@dermen
Copy link
Contributor Author

dermen commented Dec 13, 2022

Any status update here ?

@phyy-nx
Copy link
Member

phyy-nx commented Dec 15, 2022

Some folks are on leave but I think this is fine. Ima hit the auto-merge button. Thanks @dermen.

@phyy-nx phyy-nx enabled auto-merge (squash) December 15, 2022 00:19
@phyy-nx phyy-nx merged commit 48e7e37 into main Dec 15, 2022
@phyy-nx phyy-nx deleted the stills_logging branch December 15, 2022 02:14
dagewa pushed a commit that referenced this pull request Feb 20, 2023
Adds options to completely disable logging as well

* Use verbosity for SauterPoon outlier rejection.  Pull it from refinement.reflections.outlier.sauter_poon.verbose
* Add a logging handler for xfel modules
* Make suppressed the default

Co-authored-by: Aaron Brewster <asbrewster@lbl.gov>
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

3 participants