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

Allow general discrete grids #90

Merged
merged 41 commits into from
Sep 23, 2024
Merged

Allow general discrete grids #90

merged 41 commits into from
Sep 23, 2024

Conversation

timmens
Copy link
Member

@timmens timmens commented Sep 15, 2024

In this PR, I add the functionality to allow general discrete grids.

This entails two major updates:

  1. Previously, only index grids were allowed. Now an updating step is added, which takes a model containing an arbitrary discrete grid, overwrites the grid with a corresponding index grid, and adds a function to the model that transforms the index values to the correct label. This means that the user can now use arbitrary integer grids, while internally, we can assume that all grids are integer grids.
  2. We now force users to define discrete grids by instantiating DiscreteGrid with a dataclass that provides a mapping between a category name and its code. See the new syntax below.

Closes #82, #87.

New Syntax

Note

For most use cases, the codes (the field values of the dataclass) should be indices; however, with this PR it is now possible to use non-index codes; we therefore showcase this feature below

from dataclasses import dataclass
from lcm import DiscreteGrid

@dataclass
class HealthState:
   bad: int = -1
   neutral: int = 0
   good: int = 1
   
grid = DiscreteGrid(HealthState)

grid.categories  # = ["bad", "neutral", "good"]
grid.codes  # = [-1, 0, 1]
grid.to_jax()  # jnp.array([-1, 0, 1])

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.

Great start, path forward as discussed in person!

src/lcm/input_processing/__init__.py Outdated Show resolved Hide resolved
src/lcm/input_processing/process_model.py Outdated Show resolved Hide resolved
src/lcm/simulate.py Outdated Show resolved Hide resolved
src/lcm/solve_brute.py Outdated Show resolved Hide resolved
tests/input_processing/test_discrete_state_conversion.py Outdated Show resolved Hide resolved
@timmens
Copy link
Member Author

timmens commented Sep 17, 2024

Should provide only the following option to create discrete grids:

@dataclass
class RetirementStatus:
    working: int = 0
    retired: int = 1
    
DiscreteGrid(RetirementStatus)

src/lcm/grids.py Outdated Show resolved Hide resolved
src/lcm/grids.py Outdated Show resolved Hide resolved
src/lcm/grids.py Outdated Show resolved Hide resolved
@timmens timmens linked an issue Sep 19, 2024 that may be closed by this pull request
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 September 19, 2024 14:53
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.

As discussed, really nice!

examples/long_running.py Outdated Show resolved Hide resolved
explanations/function_representation.ipynb Show resolved Hide resolved
src/lcm/entry_point.py Outdated Show resolved Hide resolved
src/lcm/grids.py Outdated Show resolved Hide resolved
src/lcm/grids.py Outdated Show resolved Hide resolved
src/lcm/input_processing/discrete_state_conversion.py Outdated Show resolved Hide resolved
src/lcm/input_processing/discrete_state_conversion.py Outdated Show resolved Hide resolved
src/lcm/input_processing/discrete_state_conversion.py Outdated Show resolved Hide resolved
src/lcm/input_processing/discrete_state_conversion.py Outdated Show resolved Hide resolved
src/lcm/input_processing/discrete_state_conversion.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.

Just had a brief look -- very nice! Need to stop now.

Apologies for yet another round of renaming suggestions, but looking at it I wasn't certain that a newcomer would immediately understand what is expected where.

src/lcm/input_processing/discrete_grid_conversion.py Outdated Show resolved Hide resolved
src/lcm/input_processing/discrete_grid_conversion.py Outdated Show resolved Hide resolved
src/lcm/input_processing/discrete_grid_conversion.py Outdated Show resolved Hide resolved
src/lcm/input_processing/discrete_grid_conversion.py Outdated Show resolved Hide resolved
Copy link
Member

@ChristianZimpelmann ChristianZimpelmann left a comment

Choose a reason for hiding this comment

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

Very nice! I really see how the OOP features are beneficial.

Just a few questions or comments.

src/lcm/grids.py Show resolved Hide resolved
src/lcm/input_processing/process_model.py Show resolved Hide resolved
src/lcm/simulate.py Show resolved Hide resolved
src/lcm/grids.py 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!

return make_dataclass("CategoryClass", init)


@pytest.fixture(scope="session")
Copy link
Member

Choose a reason for hiding this comment

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

Just to check whether you really need the two functions here? I.e., with and without underscore? Can't we just decorate the function itself or does that lead to issues because it takes an argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the argument becomes a problem. While pytest supports fixtures that have arguments, it seems like you have to parametrize it via the parametrize decorator, and that would not work in our case, because we need to use different versions of it in the same test.

@timmens timmens merged commit ee48e30 into main Sep 23, 2024
7 checks passed
@timmens timmens deleted the allow-general-discrete-grids branch September 23, 2024 10:05
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.

ENH: Allow for discrete non-integer options ENH: Support arbitrary discrete integer grids
3 participants