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

ADA-SVR (2/4) add comments and documentation of the functions and test #73

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

lionelkusch
Copy link
Collaborator

@lionelkusch lionelkusch commented Dec 19, 2024

This pull request is the next step after #58 .
This is an example of a template of documentation for functions.

@lionelkusch lionelkusch changed the title ADA-SVR add comments and documentation of the functions and test ADA-SVR (2/4) add comments and documentation of the functions and test Dec 19, 2024
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.72%. Comparing base (19e6cf7) to head (c146efe).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
+ Coverage   81.70%   81.72%   +0.02%     
==========================================
  Files          43       43              
  Lines        2312     2315       +3     
==========================================
+ Hits         1889     1892       +3     
  Misses        423      423              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@bthirion bthirion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good but we should think of codebase organization.
Moreover ada_svr is too cryptic to be a good module name.

@@ -0,0 +1,51 @@
import numpy as np
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd rather not have ada_svr as a module directly under hidimstat. Should be in something like hidimstat.estimators.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine for me.
Do you prefer call this module: procedures or methods or models ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe models.
But we should first discuss the organization of the whole thing to make a good decision.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the issue #86

@lionelkusch
Copy link
Collaborator Author

I added a function for computing the pvalue from the output of ada_svr.

Avoid to modify X in the function by the line 37. 
Create new variable to compensate it.
__all__ allow to avoid to import all function when there is an import
with *.
@lionelkusch
Copy link
Collaborator Author

I add a definition of all . This is from the advice of this section.

@lionelkusch lionelkusch mentioned this pull request Dec 27, 2024
doc_conf/references.bib Outdated Show resolved Hide resolved
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