Skip to content

File cleanup: remove commented print statements and some commented code#44

Open
zurabksmlp wants to merge 4 commits intomasterfrom
file-cleanup2
Open

File cleanup: remove commented print statements and some commented code#44
zurabksmlp wants to merge 4 commits intomasterfrom
file-cleanup2

Conversation

@zurabksmlp
Copy link
Collaborator

This PR contains repository and file cleanup -- some of the unused files got removed and commented-out print statements (and occasionally other commented-out code) has been remoted in files at repo/smlp/src/smlp_py directory.

Copy link
Collaborator

@fbrausse fbrausse left a comment

Choose a reason for hiding this comment

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

Nice PR, looks good to me, thanks. Just a few questions:

Comment on lines +32 to +37
#true_pred_df = data.frame(true= bin_true_vec, pred=bin_pred_vec)
#kappa2_res = kappa2(true_pred_df, "unweighted");
#kappa2_res = kappa2(true_pred_df, "squared");
#cohen_kappa = kappa2_res$value; #pnv(cohen_kappa);
#cohen_kappa = kappa2_res$value
#true_pred_categ_df = data.frame(true=factor(bin_true_vec, levels=c(0,1)),
# pred=factor(bin_pred_vec, levels=c(0,1))); pnv(true_pred_categ_df);
# pred=factor(bin_pred_vec, levels=c(0,1)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need this commented code?

Comment on lines +46 to +47
#pos_expectation = (predicted_negative/samples_count)*true_negative
#neg_expectation = (predicted_positive/samples_count)*TP
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question: do we still need this commented code?

Comment on lines +207 to +208
#pos.scores = pos_prob_vec[bin_true_vec_pos]
#neg.scores = pos_prob_vec[bin_true_vec_neg]
Copy link
Collaborator

Choose a reason for hiding this comment

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

And again: do we need this?

solver.add(query)
res = self._modelTermsInst.smlp_solver_check(solver, 'witness_consistency')
#res = solver.check(); #print('res', res)
#res = solver.check()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove this commented line as well.

@fbrausse fbrausse changed the title File cleanup2 -- updated File cleanup: remove commented print statements and some commented code Feb 7, 2026
@zurabksmlp
Copy link
Collaborator Author

zurabksmlp commented Feb 7, 2026 via email

@zurabksmlp
Copy link
Collaborator Author

zurabksmlp commented Feb 7, 2026 via email

@konstantin-korovin
Copy link
Collaborator

konstantin-korovin commented Feb 7, 2026 via email

@fbrausse
Copy link
Collaborator

fbrausse commented Feb 7, 2026

@zurabksmlp Thanks. I did merge your previous PR #43, so now Github lets us know that there are conflicts between master and this PR #44. Could I ask you to please resolve those conflicts? You could either use the web interface or try to do that locally via git rebase master file-cleanup2.

@zurabksmlp
Copy link
Collaborator Author

zurabksmlp commented Feb 7, 2026 via email

@zurabksmlp
Copy link
Collaborator Author

zurabksmlp commented Feb 7, 2026 via email

@konstantin-korovin
Copy link
Collaborator

konstantin-korovin commented Feb 7, 2026 via email

@fbrausse
Copy link
Collaborator

fbrausse commented Feb 7, 2026

Done

OK, so there were no conflicts when rebasing, good. Please do git push -f to overwrite the remote branch (which this PR tracks) with the contents of your local branch.

@mdmitry1
Copy link
Collaborator

mdmitry1 commented Feb 7, 2026

Regression passed.
Open issues:

  1. Merge conflicts in src/smlp_py/smlp_optimize.py and src/smlp_py/smlp_terms.py
  2. Need to decide if we'd like to move from tkdiff to kdiff3 and if yes, should tkdiff->kdiff3 change be part of this pull request

@fbrausse
Copy link
Collaborator

fbrausse commented Feb 7, 2026

2. tkdiff->kdiff3 change

I'm fine with that change, don't care about the graphical tool default settings. Eventually, those should go away and get replaced with the system standard, e.g., xdg-open or similar.

@fbrausse
Copy link
Collaborator

fbrausse commented Feb 7, 2026

replaced with the system standard

Or, alternatively, these settings could become part of a ~/.smlprc or similar local config file. No need to have them hardcoded.

@zurabksmlp
Copy link
Collaborator Author

zurabksmlp commented Feb 7, 2026 via email

@fbrausse
Copy link
Collaborator

fbrausse commented Feb 7, 2026

@zurabksmlp No, that didn't work, as you can easily check on the Website: #44

Please run the same sequence of commands with an up-to-date master branch.

Copy link
Collaborator

@fbrausse fbrausse left a comment

Choose a reason for hiding this comment

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

Merging master into this branch left two commented debug prints. Due to this merge, I recommend re-running the regression suite.

@zurabksmlp
Copy link
Collaborator Author

zurabksmlp commented Feb 8, 2026 via email

@fbrausse
Copy link
Collaborator

fbrausse commented Feb 8, 2026

I do not mind removing these two print statements, but is it worth it?
Because I will need to again create a PR for file-cleanup2? And Dmitry
will need to approve again?

Yes, it is worth removing those two lines in this PR. Noone has approved it, yet, and running the regression suite should be done again anyway since the master merge did result in semantic changes, no?

You can just add a commit to the existing file-cleanup2 branch and it will be reflected in this PR. Just switch over to that branch, make sure it is in sync with the remote one on Github, add a commit that removes those two lines and push it back.

@zurabksmlp
Copy link
Collaborator Author

zurabksmlp commented Feb 8, 2026 via email

@fbrausse
Copy link
Collaborator

fbrausse commented Feb 8, 2026

Many thanks, @zurabksmlp! I'll run regressions locally again.

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.

4 participants