Open itrumper opened 1 year ago
Patch coverage: 61.62
% and project coverage change: -0.15
:warning:
Comparison is base (
ae98919
) 93.35% compared to head (39b832c
) 93.21%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Thanks a lot for this work! I apologize again to you for the very late response. Unfortunately you chose the worst possible time to issue a PR this big as I'm currently extraordinarily busy ;) Therefore I'm unfortunately unable to give a detailed review right know, but I do want to give you a bit of feedback before you invest more time into this PR.
Generally, I'm very happy to see filtering implemented in the SlogLogger; however, I'm not convinced that this approach is the way to go. A few points:
Debug
bound on Param
shouldn't be necessary (I think). I would have to look at it in detail, but the observer itself could enforce this bound without it being used all over the place in the core code. As an example you could have a look at file checkpointing, where a Serialize
bound is only required when checkpointing is actually used. I'm however not sure how well this also applies to the observer case.I had the need for filtering also in a new observer I am working on in #311. I chose to collect a list of strings and then only select the ones listed (see line 148 in this file). Note that this code essentially does something quite different to what you want to achieve, but I think the approach should be similar.
Regarding observing parameters/gradients and so on: I would love to see this, but maybe it would be easier to implement this in a dedicated observer? I'm not sure though, just an idea to think about :)
Thanks again for this contribution, I really appreciate it! I hope that not too much of your work needs to be changed and I hope that I will be able to give you a detailed review soon, but unfortunately I cannot promise it right now.
Thanks for taking the time to look it over and provide feedback! That is a great help, and thanks for letting me know about your availability. I don't have expectations of a timeline, so please don't feel pressured to work on this.
In response to your points:
Solver
s different parameters. However, I want to try to find a way that implements better type safety than string comparisons, but still is extensible. I don't know what that looks like right now, so I'll think about it more.State
but rather the Observer
itself. I only modified that file to add the enum
that defines what "names" of data are available.I'll check out the observer you are working on, thanks for the references!
For the solver specific data, like parameters and gradients, I do think it would take a different type of observer, as right now the Observer
doesn't know which parameters are available, just what is exposed through the State
trait, correct?
I'm happy to completely redo it all if a better solution is out there! You know your crate, and how pieces fit together better than I do, so I'll happily take your direction. Thanks for the discussion.
1. I see that point, we need a flexible enough interface to accommodate `Solver`s different parameters. However, I want to try to find a way that implements better type safety than string comparisons, but still is extensible. I don't know what that looks like right now, so I'll think about it more.
I agree that string comparisons aren't the most elegant solution, not only for type safety, but also for performance reasons.
2. I will certainly look into the file checkpointing! Seems like you're describing a trait bound behind a feature gate?
FileCheckpoint
is behind the serde
feature gate, but this is not what's "(de)activating" the trait bound. FileCheckpoint
imposes a Serialize
trait bound on the solver and the state (here), but only in case FileCheckpoint
is actually used. If a user doesn't use checkpointing in their code, the state and solver do not need to be serializable.
3. I may have miscommunicated about this point. Which data to log isn't stored in `State` but rather the `Observer` itself. I only modified that file to add the `enum` that defines what "names" of data are available.
I see, sorry for the misunderstanding!
For the solver specific data, like parameters and gradients, I do think it would take a different type of observer, as right now the
Observer
doesn't know which parameters are available, just what is exposed through theState
trait, correct?
That's a good point! The State
trait does expose the parameters via get_param
and get_best_param
(here). Gradients are not accessible via the State
trait, but it should be possible to create a dedicated GradientObserver
which only works with IterState
states. Then you will have access to all fields of the IterState
and it is easy to turn the observer on and off for debugging.
I found myself wanting to be able to customize what data the
SlogLogger
observer was logging, so I attempted to implement this functionality via the builder pattern as shown in the doc example in thedata
method onSlogLogger
.To do this, I needed to add the
Debug
trait as a trait bound to thetype Param
associated types, hence why this PR touches so many files. This could cause breaking changes for users.Since this uses a builder style pattern to define what data is logged, the default contains the same data as what was previously hard coded, so end users should not experience any breaking changes in their observer configurations. To customize the data that is logged, a new
enum
calledStateData
has been defined in thestate/mod.rs
file that covers all the available data you can get from theState
trait. A user supplies aVec
of these enum variants and then the logger iterates through and emits them. TheDebug
trait was needed because I wanted to print out theParam
s as well, which are generics.The motivation for doing this was to actually be able to log the gradient of a problem as well, but since not every solver has a defined gradient, I'm less sure that can be done. I will need assistance in evaluating that possibility, since it would significantly help with debugging!
Let me know what you think! Thanks for building a cool crate :)