Skip to content

Conversation

@djinnome
Copy link
Contributor

@djinnome djinnome commented Oct 21, 2024

  • Create dependency tests
  • Generate dependency tree of hierarchical parameters
  • Topologically sort dependency tree
  • Compile parameters in topologically sorted order
  • Confirm that dependency tests pass

@djinnome djinnome linked an issue Oct 21, 2024 that may be closed by this pull request
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@djinnome djinnome requested a review from Copilot January 7, 2025 23:51
@djinnome djinnome self-assigned this Jan 7, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 10 changed files in this pull request and generated 4 comments.

Files not reviewed (4)
  • docs/source/beta_mean_cycle_sir_model.json: Language not supported
  • docs/source/beta_mean_gamma_cycle_sir_model.json: Language not supported
  • docs/source/gamma_mean_beta_mean_cycle_sir_model.json: Language not supported
  • docs/source/hierarchical_sir_model.json: Language not supported
Comments suppressed due to low confidence (5)

pyciemss/mira_integration/distributions.py:50

  • Ensure that the function safe_sympytorch_parse_expr is covered by tests.
def safe_sympytorch_parse_expr(expr: SympyExprStr, local_dict: Optional[Dict[str, torch.Tensor]]) -> torch.Tensor:

pyciemss/mira_integration/compiled_dynamics.py:54

  • Ensure that param_info is of a type that get_name can handle. Consider adding a type check before calling get_name.
param_name = get_name(param_info)

pyciemss/mira_integration/compiled_dynamics.py:126

  • Ensure that param_info is correctly handled for different types in _compile_param_values_mira. Verify that the types are correctly processed for pyro.nn.PyroSample and pyro.nn.PyroParam.
param_info = src.parameters[param_name]

pyciemss/compiled_dynamics.py:24

  • The assignment of 'params' is redundant and should be removed.
params = _compile_param_values(self.src)

pyciemss/compiled_dynamics.py:33

  • Ensure that the 'get_name' function is correctly defined and used.
v = params[get_name(k)]

ParameterDict = Dict[str, Union[torch.Tensor, SympyExprStr]]


@_sort_dependencies.register(mira.modeling.Model)
Copy link

Copilot AI Jan 7, 2025

Choose a reason for hiding this comment

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

Redundant definition of sort_mira_dependencies found. This function is already defined in pyciemss/mira_integration/compiled_dynamics.py. Consider removing one of the definitions to avoid confusion.

Suggested change
@_sort_dependencies.register(mira.modeling.Model)
def safe_sympytorch_parse_expr(expr: SympyExprStr, local_dict: Optional[Dict[str, torch.Tensor]]) -> torch.Tensor:

Copilot uses AI. Check for mistakes.
expr : SympyExprStr
The sympy expression to convert to a PyTorch tensor.
local_dict : Optional[Dict]
A dictionary of free symbols and their variables to use in the sympy expression."""
Copy link

Copilot AI Jan 7, 2025

Choose a reason for hiding this comment

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

Incomplete docstring for safe_sympytorch_parse_expr. The closing triple quotes are misplaced.

Copilot uses AI. Check for mistakes.
concentration = parameters["alpha"]
else:
raise ValueError(
"MIRA InverseGamma distribution requires 'shape' or 'concentration' or 'alphaparameter"
Copy link

Copilot AI Jan 7, 2025

Choose a reason for hiding this comment

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

Typo in error message: 'alphaparameter' should be 'alpha parameter'.

Suggested change
"MIRA InverseGamma distribution requires 'shape' or 'concentration' or 'alphaparameter"
"MIRA InverseGamma distribution requires 'shape' or 'concentration' or 'alpha parameter'"

Copilot uses AI. Check for mistakes.
rate = parameters["beta"]
else:
raise ValueError(
"MIRA InverseGamma distribution requires 'rate' or 'scale' or 'beta' parameter")
Copy link

Copilot AI Jan 7, 2025

Choose a reason for hiding this comment

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

Missing period at the end of the error message.

Copilot uses AI. Check for mistakes.
@djinnome
Copy link
Contributor Author

djinnome commented Jan 8, 2025

This has been on hold until we figure out how to resolve the infinite recursion that occurs when sampling from a PyroSample object. One possible solution is to run the sympytorch module individually on each variable in topological order.

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.

Multi-level modeling support from AMRs

2 participants