Skip to content
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

Merged
merged 23 commits into from
Apr 20, 2024
Merged

Conversation

alyst
Copy link
Contributor

@alyst alyst commented Apr 20, 2024

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).

Alexey Stukalov added 2 commits April 19, 2024 22:41
for resolving the scope
* destructure lambda function args
* don't use zip(): mapreduce support multi-arg functions
@alyst alyst changed the base branch from main to devel April 20, 2024 06:00
@alyst alyst closed this Apr 20, 2024
@alyst alyst reopened this Apr 20, 2024
@alyst alyst changed the base branch from devel to main April 20, 2024 06:44
@alyst alyst closed this Apr 20, 2024
@alyst alyst reopened this Apr 20, 2024
@alyst alyst marked this pull request as draft April 20, 2024 06:56
@Maximilian-Stefan-Ernst Maximilian-Stefan-Ernst changed the base branch from main to devel April 20, 2024 09:40
@Maximilian-Stefan-Ernst
Copy link
Collaborator

I would first merge the different branches into devel, and then later merge devel -> main to make releases.

Copy link
Collaborator

@Maximilian-Stefan-Ernst Maximilian-Stefan-Ernst left a 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.

@alyst
Copy link
Contributor Author

alyst commented Apr 20, 2024

I would first merge the different branches into devel, and then later merge devel -> main to make releases.

I changed the target branch to main, because CI is not triggered for PRs to the devel branch.

Also, I am a bit confused about JuliaFormatter check during the unit test -- it looks like format() does not support the mode where it would just report the specific lines that need to be tweaked (one would need to really format and run the diff to get it). So in case of formatting issues the check just fails without giving a clue about the cause. It is still useful to have this check, but maybe it could be just turned into a warning?

alyst and others added 8 commits April 20, 2024 12:52
* 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
@alyst alyst changed the base branch from devel to main April 20, 2024 19:53
@alyst alyst closed this Apr 20, 2024
@alyst alyst reopened this Apr 20, 2024
Copy link

codecov bot commented Apr 20, 2024

Codecov Report

Attention: Patch coverage is 88.46154% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 66.09%. Comparing base (a15f92d) to head (47ae507).
Report is 30 commits behind head on main.

Files Patch % Lines
src/objective_gradient_hessian.jl 75.00% 2 Missing ⚠️
src/diff/Empty.jl 0.00% 1 Missing ⚠️
src/frontend/fit/standard_errors/hessian.jl 0.00% 1 Missing ⚠️
src/frontend/specification/RAMMatrices.jl 80.00% 1 Missing ⚠️
src/imply/empty.jl 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@alyst alyst marked this pull request as ready for review April 20, 2024 20:07
@alyst
Copy link
Contributor Author

alyst commented Apr 20, 2024

After addressing my minor comments this can be merged.

Thank you for reviewing! I have fixed those lines, the unit tests pass, so it is ready to go from my side.

@Maximilian-Stefan-Ernst Maximilian-Stefan-Ernst changed the base branch from main to devel April 20, 2024 20:58
@Maximilian-Stefan-Ernst Maximilian-Stefan-Ernst merged commit 197ce3f into StructuralEquationModels:devel Apr 20, 2024
7 of 8 checks passed
@alyst alyst deleted the fixes_1 branch April 20, 2024 21:01
@Maximilian-Stefan-Ernst
Copy link
Collaborator

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.

@Maximilian-Stefan-Ernst
Copy link
Collaborator

Maximilian-Stefan-Ernst commented Apr 20, 2024

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^^).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants