-
Notifications
You must be signed in to change notification settings - Fork 2
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
Try to update calc on init #92
Conversation
Update nodes.py
@wiep this is something we discussed briefly during the weekly on August 30. You had some reservations about the general idea to try to update a calculator on initialization - so to enrich the conversation about this proposal, I created this PR which shows how I think the update could be implemented without breaking the calculator's ability to be initialized without a working update. What do you think? |
Correct sampling for sigma_sq
Converted back to draft, because I just saw that with this implementation, we get the following warnings during model initialization:
This is not pretty, because it is just intended behavior and I think probably users should not be warned about this. Before this PR is ready for review, I would like to come up with a nicer solution for this case. |
i'm still not super happy with the default warnings. so far, we say liesel you can build a partial graph but that would trigger warnings what is not really what should happen. usually i do not like this magic, but here it might be justified: a context manager that turns the updates on (or off). i think that is better than fiddling with the log levels. with lsl.instant_evaluation():
a = lsl.Data(1.0)
b = lsl.Calc(lambda x: x / 0, a)
# warnings a = lsl.Data(1.0)
b = lsl.Calc(lambda x: x / 0, a)
# no warnings |
I don't really think the magic context manager would bring us a lot of joy here. Manually, the desired functionality can already be achieved by a = lsl.Data(1.0)
b = lsl.Calc(lambda x: x / 0, a).update() This makes it more obvious what is happening than the context manager. The idea of this PR is mostly to make it easier for users to spot errors in their code early without introducing additional manual work. If you don't know the internals of how, when and why exactly a calculator gets updated, it is currently easy to get tripped up. At least that is what happened to me. I think an alternative to the work this PR could lie in documentation.
You only need to change the log levels if you have no other way of accessing the original traceback. When you are working interactively, say in quarto, you can just run What is true is that this code looks a little fiddly: logger = logging.getLogger("liesel")
logger.handlers[0].setLevel(logging.DEBUG) This is a result of how we set up our logging. I think we could (and should) change the setup such that you can set the log level like this: logger = logging.getLogger("liesel")
logger.setLevel(logging.DEBUG) But that is a different issue. |
Co-authored-by: Hannes Riebl <[email protected]>
Building a non-initialized model is possible without warnings is imho a crucial aspect of Liesel. However, the proposed implementation may require users to turn off logging, build the non-initialized model, and then turn the logging back on. I find that not ideal. Checking the calculations during initialization is often beneficial from a user's perspective, though. Therefore, I think, it may be necessary to have two different modes to cater to both scenarios. One possible solution is to enable updates by default and disable them only when a particular setting is altered. For example, a ctx manager could be used to adjust the settings temporarily.
We can discuss this further during one of our upcoming weekly meetings. |
Co-authored-by: wiep <[email protected]>
Co-authored-by: Hannes Riebl <[email protected]>
Co-authored-by: Gianmarco Callegher <[email protected]>
* Efficient MVN Degenerate * Fixed blackjax refactor error * Refactor * indent more code * Update CHANGELOG.md --------- Co-authored-by: Gianmarco Callegher <[email protected]> Co-authored-by: Johannes Brachem <[email protected]>
We could also simply add an init parameter to calculators: a = lsl.Data(1.0)
b = lsl.Calc(lambda x: x / 0, a, update_on_init=False) This could be set to True by default. |
Allow passing key as seed
* Adds NamedTupleInterface * Format * Add documentation * Export NamedTupleInterface in goose.__init__.py * Format * Update documentation
We will go with the init argument, which defaults to |
…#167) * Transform method handles nodes duplicates * Refactor * Added tests. Removed useless check * Update CHANGELOG.md * rename and move test * rephrase message * remove unnecessary code --------- Co-authored-by: Gianmarco Callegher <[email protected]> Co-authored-by: Johannes Brachem <[email protected]>
Update nodes.py
…liesel into update-calc-on-init
@wiep if you got a notification for this PR, you can ignore it. @GianmarcoCallegher is doing the review. |
LGTM |
This is my proposal for #79
In this PR, a calculator will try to update its value during initialization. If the update fails, a warning with the title of the error is logged. Also, the full traceback of the error is logged to the debug logger.
Example 1:
Example 2: