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

Update the lut linear interpolation to be used by different lookup table files #981

Merged
merged 6 commits into from
Jan 17, 2025

Conversation

noemifrisina
Copy link
Collaborator

Split the linear_interpolation_lut into two different functions so that it can be used for lookup tables that have more than 2 columns in them (eg. DetDistConverter)

Needed for Mx#708

Instructions to reviewer on how to test:

  1. Run tests

Checks for reviewer

  • Would the PR title make sense to a scientist on a set of release notes
  • If a new device has been added does it follow the standards
  • If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
  • Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.51%. Comparing base (413aa34) to head (70f62a8).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #981   +/-   ##
=======================================
  Coverage   97.51%   97.51%           
=======================================
  Files         152      152           
  Lines        6346     6347    +1     
=======================================
+ Hits         6188     6189    +1     
  Misses        158      158           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@noemifrisina noemifrisina marked this pull request as ready for review January 10, 2025 15:49
@noemifrisina noemifrisina requested a review from a team as a code owner January 10, 2025 15:49
@noemifrisina noemifrisina requested a review from rtuck99 January 10, 2025 15:49
def linear_interpolation_lut(filename: str) -> Callable[[float], float]:
def parse_lookup_table(filename: str) -> list[Sequence]:
"""Parse a generic lookup table with a number of columns >= 2 and return a list \
of all the values in it."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you reword this - the description makes it sound as though it returns a list in row major order (which is what you'd naively expect) but it actually returns it in column-major order.

)
s_values = list(reversed(s_values))
t_values = list(reversed(t_values))
if not np.all(np.diff(s_values) > 0):
raise AssertionError(
f"Configuration file {filename} lookup table does not monotonically increase or decrease."
"Configuration file lookup table does not monotonically increase or decrease."
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a file anymore, it's just a table

Copy link
Contributor

@rtuck99 rtuck99 left a comment

Choose a reason for hiding this comment

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

Good but I think can you do something to make the error messages nice again - it's a pet hate of mine when something raises an error about a file but then doesn't tell you which file it is!

Perhaps just wrap the outer parse function like:

def lookup_table_from_file(filename: str,
                           lookup_for_columns: Callable[[Sequence], Callable[[float], float]]) -> Callable[[float], float]:
    try:
        columns = parse_lookup_table(filename)
    except Exception as e:
        # log message here
        raise
    return lookup_for_columns(*columns)

However this may heading down the road of too much boilerplate for some, so I leave it up to you if you want to do this or not.

@noemifrisina noemifrisina merged commit a822a40 into main Jan 17, 2025
17 checks passed
@noemifrisina noemifrisina deleted the mx_684-det-center-lut branch January 17, 2025 10:38
stan-dot pushed a commit that referenced this pull request Jan 17, 2025
…ble files (#981)

* First idea for beam center, may move compute to plans

* Actually make it a plan

* Fix the tests

* Tidy up docstring

* Reword comments
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.

2 participants