-
Notifications
You must be signed in to change notification settings - Fork 6
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
Misc. fixes #196
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. |
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.
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. |
197ce3f
into
StructuralEquationModels:devel
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^^). |
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).