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

Relax Julia compat version and solve_residuals! method #65

Merged
merged 5 commits into from
May 7, 2024

Conversation

junyuan-chen
Copy link
Contributor

@junyuan-chen junyuan-chen commented Apr 10, 2024

Thank you working on this package! I have two issues that I would wish to be resolved here.

  1. It seems strange that somehow the minimum Julia version was changed to v1.9 with Add Metal and CUDA as extensions #56. This seems unnecessary to me and also undesirable. With FixedEffectModels.jl now requiring FixedEffects.jl v2.3, all packages that directly or indirectly deals with fixed effects now requires Julia v1.9 to be paired with the latest versions of the two. This sounds way too much at least for the recent years.

  2. A related issue is that the type restrictions for solve_residuals! seem to be too stringent. I would suggest having a fallback method as what I have did here that does not restrict the type of xs at all. If something wrong is passed to the method, then an error will be thrown anyway when the loop inside this fallback method iterate over xs and call other methods of solve_residuals!.

Edit: I see that Metal.jl would cause some problem. But, the GPU part could be ignored if the Julia version is too old.

@junyuan-chen
Copy link
Contributor Author

junyuan-chen commented Apr 11, 2024

@matthieugomez Sorry for being pushy here. But, hopefully it could be quick for you to take a look at this. I have some releases to make elsewhere where I would rather not switch the minimum version to v1.9. Thank you!

@matthieugomez
Copy link
Member

I'd be willling to have a less stringent signature but can you give me an example of stuff passed to solve_residuals which is not an abstractvector of abstract vector? You should probably explain that in the code so that no one undoes it by mistake

@junyuan-chen
Copy link
Contributor Author

@matthieugomez There are two scenarios where I encountered some issues. First, the columns may come from an iterator that is not an AbstractVector. In that case, I ended up circumventing the issue with a loop like this below:
https://github.com/JuliaDiffinDiffs/DiffinDiffs.jl/blob/master/lib/InteractionWeightedDIDs/src/utils.jl#L43-L58

A second scenario is that the columns were collected in a tuple.

In retrospect, I think this is not a big deal as I can still add a loop to iterate column by column without dealing with the method for vectors of vectors. However, I still feel the type restrictions that the container for the columns have to be a vector unnecessarily restrictive.

@matthieugomez
Copy link
Member

matthieugomez commented May 6, 2024

OK thanks. Could please add "the columns may come from an iterator that is not an AbstractVector" etc and the reference of your package as a comment next to the function definition + tag a new minor version 2.4.0? Then I'll merge your PR

@junyuan-chen
Copy link
Contributor Author

@matthieugomez Should be good now.

@eloualiche eloualiche merged commit 5c2e4df into FixedEffects:master May 7, 2024
2 checks passed
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.

3 participants