Skip to content

BUG: make Presets.status more tolerant of unknown/uninitialized positions#1319

Merged
tangkong merged 3 commits intopcdshub:masterfrom
tangkong:bug_preset_error
Jan 24, 2025
Merged

BUG: make Presets.status more tolerant of unknown/uninitialized positions#1319
tangkong merged 3 commits intopcdshub:masterfrom
tangkong:bug_preset_error

Conversation

@tangkong
Copy link
Contributor

Description

Makes Preset Preset.device.wm_* and Presets.state methods more tolerant of missing or uninitialized device positions.

This changes the signature of the wm_* methods slightly to also return "Unknown" if the position cannot be resolved.

Motivation and Context

closes #1236

As these are autogenerated methods, it's a little harder to confirm that this return type change won't break downstream consumers. At least within pcdsdevices, the only method that calls a device.wm_* method is Presets.state. It's possible that some script very strictly requires the output of a preset to be a numeric, but if the position was uninitialized, those scripts would throw exceptions anyway (no return would be reached)

How Has This Been Tested?

Checking that CI passes.

By manually setting device.postition to None and seeing the output not explode

In [1]: xcs_dg3_pim.y.position
Out[1]: -51.99951171875

In [2]: xcs_dg3_pim.y.wm_ccm()
Out[2]: 59.49951171875

In [3]: xcs_dg3_pim.y.presets.state()
Out[3]: 'Unknown'

In [4]: xcs_dg3_pim.y._position = None

In [5]: xcs_dg3_pim.y.wm_ccm()
Out[5]: 'Unknown'

In [6]: xcs_dg3_pim.y.presets.state()
Out[6]: 'Unknown'

Where Has This Been Documented?

This PR

Pre-merge checklist

  • Code works interactively
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

wm_state = getattr(device, method_name)
diff = abs(wm_state())
state_val = wm_state()
if not isinstance(state_val, (int, float)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't ever get numpy floats in these, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure, but it's probably prudent to future-proof ourselves. I thought about this for half a second then forgot about it. numbers.Number should work.

In [5]: for n in [1, 1.2, np.int32(3.3), np.float64(3.2)]:
   ...:     print(isinstance(n, numbers.Number))
   ...: 
True
True
True
True

pos = preset_pos - curr_pos
except Exception:
# current position is not known or unset
pos = "Unknown"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach is definitely better than the status quo, but I have to wonder if a cleaner approach would have been to make a custom error message.

It's not clear to me.

At any rate, these are intended more for interactive behavior at beamlines, so eschewing an exception for a str output will often be appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my reasoning here as well. Thinking about the actual issue that spawned this bug report (overloaded callback queue delaying position update), an uninitialized position could be caused by a multitude of causes. I think it might be hard to provide anything more useful than the "position is unknown" in an error message

@tangkong tangkong requested a review from ZLLentz January 24, 2025 18:55
@tangkong tangkong merged commit f60dfba into pcdshub:master Jan 24, 2025
11 checks passed
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.

BUG: preset formatting should be more error tolerant

2 participants