fix bug that mistakenly checks for positive ndts on missing data#782
Merged
AlexanderFengler merged 20 commits intomainfrom Sep 26, 2025
Merged
fix bug that mistakenly checks for positive ndts on missing data#782AlexanderFengler merged 20 commits intomainfrom
AlexanderFengler merged 20 commits intomainfrom
Conversation
cpaniaguam
requested changes
Aug 19, 2025
Collaborator
cpaniaguam
left a comment
cpaniaguam
left a comment
There was a problem hiding this comment.
Just a few observations to consider for readability and maintainability:
- Possibly refactor some conditionals and logic for clarity
- Could be worth adjusting logging to defer string formatting
src/hssm/distribution_utils/dist.py
Outdated
Comment on lines
+786
to
+789
| if params_is_reg is not None: | ||
| params_only = True if (not any(params_is_reg)) else False | ||
| else: | ||
| params_only = False |
Collaborator
There was a problem hiding this comment.
Suggested change
| if params_is_reg is not None: | |
| params_only = True if (not any(params_is_reg)) else False | |
| else: | |
| params_only = False | |
| params_only = params_is_reg is not None and not any(params_is_reg) |
| if missing_data and not deadline: | ||
| network = MissingDataNetwork.CPN | ||
| elif not missing_data and deadline: | ||
| elif missing_data and deadline: |
Collaborator
There was a problem hiding this comment.
There is a change in logic here. Is it intentional?
src/hssm/hssm.py
Outdated
Comment on lines
1924
to
1933
| if self.model_config.backend != "pytensor": | ||
| missing_data_callable = make_missing_data_callable( | ||
| self.loglik_missing_data, "jax", params_is_reg, params_only | ||
| self.loglik_missing_data, "jax", params_is_reg, None | ||
| ) | ||
| else: | ||
| missing_data_callable = make_missing_data_callable( | ||
| self.loglik_missing_data, | ||
| self.model_config.backend, | ||
| params_is_reg, | ||
| None, |
Collaborator
There was a problem hiding this comment.
Why not determine the backend first and keep a single call to make_missing_data_callable? It could look like this:
backend = "jax" if self.model_config.backend != "pytensor" else self.model_config.backend
missing_data_callable = make_missing_data_callable(
self.loglik_missing_data,
backend,
params_is_reg,
None,
)
Comment on lines
+1946
to
+1950
| _logger.info( | ||
| f"Re-arranging data to put split missing and observed datapoints. " | ||
| f"Missing data (rt == {self.missing_data_value}) will be on top, " | ||
| f"observed datapoints follow." | ||
| ) |
Collaborator
There was a problem hiding this comment.
Suggested change
| _logger.info( | |
| f"Re-arranging data to put split missing and observed datapoints. " | |
| f"Missing data (rt == {self.missing_data_value}) will be on top, " | |
| f"observed datapoints follow." | |
| ) | |
| _logger.info( | |
| "Re-arranging data to put split missing and observed datapoints. " | |
| "Missing data (rt == %s) will be on top, observed datapoints follow.", | |
| self.missing_data_value | |
| ) |
digicosmos86
approved these changes
Sep 2, 2025
Collaborator
digicosmos86
left a comment
digicosmos86
left a comment
There was a problem hiding this comment.
LGTM! I have forgotten what conversations we had about go-nogo and how to branch different use cases, but as far as this PR goes, this looks good
|
|
||
| @pytest.mark.slow | ||
| @pytest.mark.parametrize(parameter_names, parameter_grid) | ||
| @pytest.mark.parametrize(PARAMETER_NAMES, PARAMETER_GRID) |
Collaborator
There was a problem hiding this comment.
This is more a stylistic choice, but why we are using all caps for variables that are not constants?
Member
Author
There was a problem hiding this comment.
hm from my perspective these are global constants, defined up top.
weird case.
tests/test_data_sanity.py
Outdated
| match="You have no missing data in your dataset, " | ||
| + "which is not allowed when `missing_data` or `deadline` is set to " | ||
| + "True.", | ||
| match=r"Missing data is provided as True, " |
Collaborator
There was a problem hiding this comment.
Maybe use missing_data to signify that it is an argument?
| ) | ||
| with pytest.raises( | ||
| ValueError, | ||
| match="Missing data provided as False. \n" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes an issue where we mistakenly check for
ndt - rt > 0for missing data, which wrongly set the log likelihood to the lower bound in those cases.