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.

Sign up for GitHub

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

Jump to bottom

add em and draft nll #29

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

add em and draft nll #29

wants to merge 27 commits into from

Conversation

PBrblt
Copy link
Contributor

@PBrblt PBrblt commented Jun 30, 2022

No description provided.

@anujanegi
Copy link
Member

@PBrblt Thanks for adding your method!
can you also test your method by adding it to test_estimators.py.
Additionally, you can also think of adding NLL as a testcase in the above file. Can also add it for benchmarking in benchmark.py, you might have to be careful with the arguments that you pass/use, for eg. active set is not calculated in benchmark.py, I would suggest calculating it inside the NLL function by calling _get_active().

anujanegi
anujanegi reviewed Jul 5, 2022
View reviewed changes
bsi_zoo/metrics.py Outdated Show resolved Hide resolved
PBrblt and others added 2 commits July 5, 2022 12:29
@PBrblt
include em and nll to benchmark
affed29 
First attempt at including the new method/metric to the benchmark
@anujanegi
Merge branch 'master' into em_estimator
5c909b5
@anujanegi anujanegi marked this pull request as ready for review July 6, 2022 08:23
anujanegi and others added 2 commits July 6, 2022 10:24
@anujanegi
Merge branch 'master' into em_estimator
0e5b13f
@PBrblt
correcting the update
8291250 
In addition improved some part of the code.
@anujanegi anujanegi requested a review from agramfort July 11, 2022 12:51
agramfort
agramfort reviewed Jul 11, 2022
View reviewed changes
Copy link
Collaborator

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

@PBrblt @anujanegi can you make the CIs green before considering a merge ? 🙏

bsi_zoo/metrics.py Outdated Show resolved Hide resolved
@anujanegi
Copy link
Member

anujanegi commented Jul 18, 2022 •
edited
Loading

@PBrblt I updated the test cases to take less time to run.
Can you check you method, all tests fail. The estimates returned are filled with Nan values.

anujanegi
anujanegi reviewed Jul 22, 2022
View reviewed changes
bsi_zoo/tests/test_estimators.py
@@ -28,6 +29,7 @@
(iterative_L1_typeII, 0.1, 1e-1, 5e-1, "full"),
(iterative_L2_typeII, 0.1, 1e-1, 1e-1, "full"),
(gamma_map, 0.2, 1e-1, 5e-1, "full"),
(lemur, 0.2, 1e-1, 5e-1, "diag"),
Copy link
Member

Choose a reason for hiding this comment

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

rtol and atol values are chosen by you @PBrblt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agramfort agramfort agramfort left review comments

@anujanegi anujanegi anujanegi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PBrblt @anujanegi @agramfort