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

Disabling plots in lineup plan #818

Open
canismarko opened this issue Feb 20, 2023 · 5 comments
Open

Disabling plots in lineup plan #818

canismarko opened this issue Feb 20, 2023 · 5 comments

Comments

@canismarko
Copy link
Collaborator

The lineup plan (apstools.plans.alignment.lineup) uses a best effort callback to capture results, either through the bec argument or from the ipython shell namespace. Either way, it calls bec.enable_plots(). This is a sensible default, but caused problems for me in using this plan in a CI environment.

I could see two remedies:

  1. Add an argument to the plan that determines whether enable_plots gets called (e.g. def lineup(..., enable_plots=True))
  2. Only call bec.enable_plots() if the bec comes from the ipython shell namespace, allowing me to use bec.disable_plots() before passing the bec as the bec argument to lineup().

The first one seems simpler to me, but happy to submit a PR for either one.

@prjemian
Copy link
Contributor

The only way the present implementation of lineup can get any peak statistics is by enabling bec plots. It uses the PeakStats class.

I'd be happy dropping the dependence on bec completely and switching to use PySumReg. We should be able to lineup whether or not plots are drawn.

@canismarko
Copy link
Collaborator Author

Ah, gotcha. Strange that peakstats requires plotting to be enabled. I'm fine with using pysumreg, though I don't think I have time the bandwidth to implement that right now. I have a workaround for my tests so it's okay for now.

@prjemian
Copy link
Contributor

I believe that PeakStats relies on methods in BestEffortCallback to identify the plottable signals. To break the binding, those methods would have to be refactored out of BEC. Current rate of development suggests to me that is not likely in time frame of years.

@prjemian
Copy link
Contributor

Unless you disagree, I will close this issue as wontfix. For me, the apstools.plans.lineup2() plan is the replacement that works regardless of plotting support. It uses PySumReg.

@prjemian
Copy link
Contributor

#884

@prjemian prjemian added this to the 1.7.1 milestone Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants