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 function evaluator #81

Merged
merged 27 commits into from
Jun 27, 2024
Merged

Update function evaluator #81

merged 27 commits into from
Jun 27, 2024

Conversation

timmens
Copy link
Member

@timmens timmens commented Jun 14, 2024

In this PR, I update the code corresponding to the function evaluator.

  • Add typing to function_evaluator.py module
  • Replace all occurrences of non-numerical discrete labels. (We will not support them.)
    For example, we test the lookup with characters ["good", "bad"], which we do not want to support.
  • Replace all occurrences of numerical labels that are not themselves indices (i.e. [0, 1, ..., endpoint] is okay, but [1, 2] is not)
  • Rewrite label translator to only return identity function (works on the assumption that all discrete labels that are processed internally are their own indices)
  • Throw error when processing a model with a discrete grid that does not correspond to indices
  • Add explanation notebook on function evaluator
    • Write motivation presenting the high-level function
    • Write technical section on behavior of small functions
    • Add task to run explanation notebooks on CI (prevent misaligned of LCM codebase and these notebooks)
  • Compare map_coordinates to other interpolation tools, e.g., RegularGridInterpolator
    • It turns out that the RegularGridInterpolator implements a linear extrapolation feature. However, it is prohibitively slower compared to map_coordinates. Luckily, we can adjust map_coordinates to also perform linear extrapolation. See notes below.

Note

  1. This PR introduces a new error that will be thrown during the model processing. It was already discouraged to use discrete grids that are not indices (since other grids were not implemented and would throw an uninformative error). Now, an informative error is thrown if the user tries to use a grid that does not coincide with indices.
  2. During work on this PR, Janos and I discussed options to implement arbitrary discrete grids. We settled on a solution that is described in ENH: Support arbitrary discrete integer grids #82. We do not implement it here, because it will be easier to implement once the model processing has been refactored.
  3. While I found a very nice solution to add extrapolation to our function evaluator, we will not tackle any extrapolation issues in this PR since we have to wait for a response from a JAX maintainer; see issue ENH: Add linear extrapolation to map_coordinates  #83 for details.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@timmens timmens requested a review from hmgaudecker June 16, 2024 17:25
Copy link
Member

@hmgaudecker hmgaudecker left a comment

Choose a reason for hiding this comment

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

Looks great! There may be room for language improvement, but mainly about docs. I already changed a little bit.

explanations/function_evaluator.ipynb Outdated Show resolved Hide resolved
explanations/function_evaluator.ipynb Outdated Show resolved Hide resolved
explanations/function_evaluator.ipynb Outdated Show resolved Hide resolved
explanations/function_evaluator.ipynb Outdated Show resolved Hide resolved
explanations/function_evaluator.ipynb Outdated Show resolved Hide resolved
src/lcm/function_evaluator.py Outdated Show resolved Hide resolved
src/lcm/function_evaluator.py Outdated Show resolved Hide resolved
src/lcm/function_evaluator.py Outdated Show resolved Hide resolved
Copy link
Member

@hmgaudecker hmgaudecker left a comment

Choose a reason for hiding this comment

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

Excellent, thanks!

src/lcm/function_representation.py Outdated Show resolved Hide resolved
@timmens timmens merged commit 59d81aa into main Jun 27, 2024
7 checks passed
@timmens timmens deleted the update-function-evaluator branch June 27, 2024 12:02
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