-
Notifications
You must be signed in to change notification settings - Fork 4
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
Introduce detailed calibration implementation #76
base: main
Are you sure you want to change the base?
Conversation
The current version still includes some preliminary implementation bits required to apply the calibration testing data. This will be removed for the final version. Also, I would still like to do some small tests, but I will not change the structure anymore. Therefore it would be very helpful to already hear your views @robinhasse. |
c062bdf
to
b0d82a6
Compare
I've completed the final tests and fixed some bugs, so the PR is now ready for review. Some preliminary code bits remain (e.g. the gams switch |
cb4e9ec
to
e865271
Compare
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.
Wow, very impressive work! Apart from sume in-line suggestions, I have some comments, questions:
- The two calibration procedures share so much code that I would consider one function with if clauses to avoid code duplication. I'm sure you have thought about this. What is the reason you decided against it?
- Similar argument inside each calibration method function for iteration 0 and the rest. There seems to be a lot of shared code. Don't you think you could run a for loop starting with 0 and differentiate 0 from the rest where needed?
- I have to admit I didn't dig deep into the step size computation but I see correctly that you need to run BRICK several times just to adjust the step size?
- You have several objects where you name the value columns differently (e.g. target) but need to rename it to value before passing it to gams eventually. If this is not needed I'd avoid it.
- I'm not familiar with gams macros. Why did you reformulate the parallel solving to a macro? Is it just a more compact formulation, does it have any other benefits or was it needed for the calibration?
Many thanks overall for this very comprehensive development @ricardarosemann. Nice work!
Thank you for all your comments, those are very good points!
Yes, I thought about this - my thought was that this way the code for each procedure is clearer and more readable and I decided to accept the code duplicates then. Especially within the for-loop there are actually quite some differences, so you'd need two or three large if-clauses. But I'm very open to discussion here - I'd say neither solution is optimal.
I actually don't quite see this point - in case of the logit procedure, there is not much shared code, as in later iterations Brick is only run within the step size algorithm and all required results are taken from the last evaluation in the step size algorithm. In case of the optimization procedure there are some more shared lines. But since in iteration zero not much happens, you'd either need a large a if-clause covering most of the for-loop, or the call to Brick would need to be moved to the beginning of the loop. But then the Brick-call would be separated from the procedure that generates the respective input data, which I don't find so intuitive.
Yes, that's correct. Although it does happen quite often that the originally chosen step size already satsifies the conditions, so it is in fact only run once. The idea is that choosing an appropriate step size will reduce the number of calibration iterations considerably, which in the case of the optimization procedure is much more costly (as for the step size adaptation we only need a normal Brick run, whereas in each calibration iteration we do a run where we compute the gradient, corresponding to numerous normal Brick runs). In case of the logit calibration, the step size adaptation can avoid divergence of the procedure. So my conclusion would be that it's worth the effort.
The reason for this usually is that I want to join the object with another one and manipulate the data. And I think that if that happens somewhere in the code, I decided to do the renaming once when I generate the object and redo this directly before passing to gams. Of course I could also only do the renaming directly before joining and return to the standard "value" as soon as possible. As many objects actually have several data columns, which of course need a distinct name, I find it a bit more convenient and consistent the way I did it. But we could also change this if you prefer.
It saves me from repeating the same code bit several times: All that the macro does is to copy the code included in the macro to the location where I call it (I think this happens during compilation). It was very necessary with the old calibration implementation, where I called it even more often. Currently, I use the macro in four locations (one for the standard brick run, three for the optimization calibration), so I'd still think it makes sense. |
…ion procedure in runCalibration.R; major restructuring in comparison to test-calibration branch.
e865271
to
4367acb
Compare
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.
Great work, thanks @ricardarosemann!
Two calibration procedures are available:
Both calibration procedures make use of a step size adaptation. The main algorithm that is used is the Armijo step size adaptation algorithm originating from numerical optimization procedures (such as the one used in the optimization calibration). In case of the logit calibration, this is not always applicable. Then a very rough heuristic algorithm is applied.
The calibration is mainly handled in the R-function
runCalibration
. The logit calibration only calls standard Brick Gams runs. The optimization calibration partially requires a modified Brick Gams routine.