search

NSLS-II-SIX / sixtools

Software for performing resonant inelastic xray scattering analysis at NSLS-II
https://pypi.org/project/sixtools
Other
1 stars 2 forks source link

Preview plans #32

Closed awalter-bnl closed 4 years ago

awalter-bnl commented 5 years ago

This address bluesky issue #1075 raised by @ambarb . I have created a 'custom' version of hat gives an output very similar to that described in bluesky issue #1075 . The printed output has the form: "scan no XXXX, 'reason', motor1 = Y1, motor2 = Y2, ........" where XXXX is the expected scan ID, 'reason' is an optional string given by the 'reason' kwarg in the scans metadata and Yi is the value that motori was set too prior to running the scan.

NOTES: 'reason' is ignored if it is not present as a kwarg in the metatdata, only motors that are 'set' between 'scans' are recorded.

To get the equivalent output requested in bluesky issue #1075 the kwarg 'reason' would need to be set to 'sample 3 A, pol = V' or 'dark' depending on the scan.

Based on discussion with @ambarb I have also included a 'check_plan' function which combines all of the currently offered 'simulators'. In this by default is ignored.

awalter-bnl commented 5 years ago

The Travis failure here is because 'db' is not defined as this is defined when running bsui I think I don't need to define it here. At any rate defining it here becomes problematic, as I would need to ensure it is defined the same as when running bsui.

tacaswell commented 5 years ago

This well never see the db defined in the user namespace.

I think it is dangerous to show users 'estimated' scan ids. If they are usually right users may start writing these predictions down. If something happens and the predictions are wrong (ex, the run an extra count before they really run the scan!) their notes will now be off.

I think the best solution is to put is some dummy place holder, but if they insist on giving their users this foot-cannon then this function needs to take in a broker and then in their profile using partial to bind their db to it.

awalter-bnl commented 5 years ago

Based on a discussion with @tacaswell I have included 'db' as an argument to the 2 plans, and a corresponding partial reference to these plans should be included in the startup files

codecov-io commented 5 years ago

Codecov Report

Merging #32 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #32   +/-   ##
=======================================
  Coverage   43.26%   43.26%           
=======================================
  Files           3        3           
  Lines         282      282           
=======================================
  Hits          122      122           
  Misses        160      160

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f7091ac...3cdb38f. Read the comment docs.

danielballan commented 5 years ago

Would it be more reliable to consult RE.md.get('scan_id', 1) and pass RE in instead of db? I share @tacaswell's fear of implementing things are just usually right. This still has that problem, but is in my opinion more likely to be correct.

awalter-bnl commented 5 years ago

I am happy to go either way, I don't think the major source of error is that we are getting it from databroker instead of the run engine. To me the major source of errors is that it assumes that the first scan ID is the next scan ran by the run engine, and if there are failed/restarted scans or other such issues it is no longer valid.

So let me know if you are insisting @danielballan, or if anyone else prefers @danielballan 's suggestion , and I will make the change.

danielballan commented 5 years ago

Don't feel strongly about RE vs db. The larger point worries me, but if the users are unpersuaded I won't block merging.

mpmdean commented 5 years ago

Maybe print a more verbose message to the users. (Unfortunately, most won't pick up on the docustring. )

Scan 0 in plan (likely scan_id 12432)

or

Scan 0 in plan (scan_id 12432 assuming plans starts as subsequent scan_id)

awalter-bnl commented 5 years ago

@mpmdean this is a discussion for the beamline staff. I have used the message layout that is currently printed at the completion of the scan, which I think is what was asked. @bisogni or @ambarb do you have a prefered format for the printed message?

awalter-bnl commented 5 years ago

@ambarb and @bisogni Just a friendly reminder that this is awaiting your feedback so that it can be merged.

awalter-bnl commented 4 years ago

Closing this PR as I assume that after ~ 1 year from the last request for feedback and no repsonse it is no longer needed.

mpmdean commented 4 years ago

The issue isn't really a lack of feedback on this issue, but that the development of these sixtools has stopped completely.

ambarb commented 4 years ago

For this particular bit that started this entire discussion, those plans are not run very often at SIX now because we are supporting users.

My hope in putting the issue in bluesky was to develop a general solution that would make print_summary() act more like a simulator as this is a functionality that I feel is lacking. I was using the plan at 2ID to illustrate the point. However, on the bluesky issue, there is no one with my viewpoint so why should anyone invest the time in making a proper simulator?