-
Notifications
You must be signed in to change notification settings - Fork 31
Fix #1006 #1031
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
Fix #1006 #1031
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, thanks!
|
|
||
| @fail_function( | ||
| include_if_any_element_present=[ | ||
| "specialized_environment__tax_transfer_dag", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "specialized_environment__tax_transfer_dag", | |
| "specialized_environment__tax_transfer_function", |
My bad in the issue description. The DAG only requires the labels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this PR is just done with this change:
@fail_function(
include_if_any_element_present=[
-> "raw_results__columns",
]
)
def root_nodes_are_missing(
I think it's just that the root_nodes_are_missing triggers to early. It shouldn't do
when just using specialized_environment__tax_transfer_dag as a target, and maybe it
also shouldn't when we target specialized_environment__tax_transfer_function.
Do we need input data for anything else than num_segments when creating the DAG or the
TT function? Both should be fine without, no? We could use the default for
num_segments then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that's it. Let's not worry too much.
The num_segments needs to be at least a large as the number of observations. But it is Jax-specific and we cannot check everything -- the probability that someone creates a function with some input data and then injects other data with Jax seems sufficiently small. Sort of similar to #966. If you feel like it, create an issue, but nothing to worry about now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I did not immerse myself enough into this when replying above -- was thinking that the root nodes in root_nodes_are_missing were those of the interface DAG 🙈
- Renamed to
fail_if.tt_root_nodes_are_missing - Changed the behaviour s.t. we send a different message when
processed_datais empty - Removed a test so that we continue to be a bit stricter (less code, less special cases)
Please double-check and merge as you see fit!
tests/ttsim/test_end_to_end.py
Outdated
| assert flat_result_template.keys() == flat_expected.keys() | ||
|
|
||
|
|
||
| def test_can_create_tt_function(backend: Literal["numpy", "jax"]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note that this is precisely what we don't want, as the num_segments will be potentially wrong. Certainly we don't want a test for this.
|
Okay, thanks! RE creating TT function without input data: The more intuitive approach for me would be to ask the user to provide
But you put far more thought into this than I did, so I might be missing something! |
|
We also need the data for checking scalar inputs -- these will be put into the |
e0d9801
into
collect-components-of-namespaces
What problem do you want to solve?
Closes #1006