Misc. fixes#196
Conversation
for resolving the scope
* destructure lambda function args * don't use zip(): mapreduce support multi-arg functions
|
I would first merge the different branches into devel, and then later merge devel -> main to make releases. |
Maximilian-Stefan-Ernst
left a comment
Maximilian-Stefan-Ernst
left a comment
There was a problem hiding this comment.
After addressing my minor comments this can be merged.
I changed the target branch to Also, I am a bit confused about JuliaFormatter check during the unit test -- it looks like |
avoids full evalulation when the end result is known
use dict for faster observed-to-specified matches
* introduce ArrayParamsMap type for clarity * annotate types of the corresp. RAMMatrices fields * remove cartesian indexing since it is not used * rename get_parameter_indices() into array_parameters_map() * use the single pass over the array for performance
to match julia naming convention
no declarations, so import is not required
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #196 +/- ##
==========================================
- Coverage 67.10% 66.09% -1.02%
==========================================
Files 51 51
Lines 2490 2548 +58
==========================================
+ Hits 1671 1684 +13
- Misses 819 864 +45 ☔ View full report in Codecov by Sentry. |
Thank you for reviewing! I have fixed those lines, the unit tests pass, so it is ready to go from my side. |
|
Nice, good changes! I wanted to have Formatter in the unit tests because I recently made some PRs to another repo, and they had it in their unit tests, which is quite nice because I would have forgotten a thousand times to format before opening the PR if it did not remind me - but having it as a warning is a bit nicer, I agree. W.r.t. to the tests when opening a PR to devel: the way I intended it is that the "CI" should run instead of "CI_extended" - I will check if/why this does not work. |
|
Ah, fixed the CI - now, when opening a PR to devel, the "CI" should run. The only real difference to "CI_extended" is that it only runs on Ubuntu and it also constructs the tested models piece by piece, if I remember correctly - so I thought the normal CI is enough for developing, only when something should be merged into main to make a release, we test on every OS (because normally I don't want to wait the eternity it takes to test on MacOS^^). |
2 participants
This is the first round of cherry-picking from #193: mostly typo fixes and some cosmetic cleanups.
Many of these changes, in particular around RAMMatrix and objective/gradient/hessian evaluation, would be later superseded by more substantial refactorings (they should be easier to cherry-pick into future PRs, once this one is landed).