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

Feedback before Code Review 2 #28

Closed
adrhill opened this issue Jun 25, 2024 · 3 comments
Closed

Feedback before Code Review 2 #28

adrhill opened this issue Jun 25, 2024 · 3 comments

Comments

@adrhill
Copy link

adrhill commented Jun 25, 2024

Hi all,

here's some quick feedback before the second code review session next week.
I'm opening this as a single issue to not spam your repository.

Documentation

  • Please describe in the README what the repo is about and that it is part of course work at TU Berlin
  • Please build a doc page until code review session 2 and include a usage example

Code

  • Good job on adding compat entries for all of your dependencies!
  • It's good practice to include all of your source files, all of your dependencies and all of your exports in your "main file" src/JML_XAI_Project.jl. An example is given here.
    You don't need to add more modules inside of your package.
  • Your LIME struct could use a docstring
  • Docstrings need to be written directly above methods and structs, without a newline (here in L36):
    """
    normalize_data(X::Matrix, y::Vector, weights::Vector)
    Returns the weight normalization of X and y using the weight vector y.
    X_norm = ((X - np.average(X, axis=0, weights=weights)) * np.sqrt(weights[:, np.newaxis]))
    Y_norm = ((y - np.average(y, weights=weights)) * np.sqrt(weights))
    """
    function weighted_data(X, y, weights)
    #TODO
    return nothing, nothing
    end
@e-strauss
Copy link
Owner

thanks @adrhill , we will look into that.

I have question regarding the unregistered LARS algorithm package we depend on. I looked into other lasso path packages, but they result in a different Coefficient - matrix, which is not suitable from my perspective. So we would stick to the LARS repo for now. I actually I needed to create my own fork of the repo, fix some function renaming to make julia 1.10 compatible...

@adrhill
Copy link
Author

adrhill commented Jun 26, 2024

So we would stick to the LARS repo for now.

Sounds good!

I actually I needed to create my own fork of the repo, fix some function renaming to make julia 1.10 compatible...

Bonus points if you make a PR to the original repo with your fixes! ;)
You can run CI on the latest Julia release (currently 1.10) by updating these lines in LARS.jl to:

- '1.0'
- '1.6'
- '1' # latest release

However, it looks like a similar PR might already exist: simonster/LARS.jl#10

@e-strauss
Copy link
Owner

thanks - fixed in #51 & #61

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

No branches or pull requests

2 participants