Visualization fw neel more by konstantin-korovin · Pull Request #18 · SMLP-Systems/smlp

konstantin-korovin · 2025-06-11T18:17:52Z

No description provided.

Bench low dim

Th0ught09

A lot of comments but I'm not sure if they're intentional or were left in as a mistake. Not too sure of the general purpose of the pr par for adding some better output and formatting

Th0ught09 · 2025-10-20T17:16:56Z

src/smlp_py/smlp_models.py

 from smlp_py.train_caret import ModelCaret 
 from smlp_py.train_sklearn import ModelSklearn
 from smlp_py.smlp_utils import str_to_bool
+#from smlp_py.smlp_utils import str_to_bool, model_features_sanity_check


best to delete comment

Suggested change

#from smlp_py.smlp_utils import str_to_bool, model_features_sanity_check

Th0ught09 · 2025-10-20T17:18:40Z

src/smlp_py/train_keras.py

            else:
                self._keras_logger.info('dense layer of size ' + str(size))
                model.add(keras.layers.Dense(units=size, activation=hid_activation))
+                #model.add(keras.layers.Dropout(rate=0.2))


I see a lot of these lines added only to be commented out, is this a mistake or intentional?

If the PDF documentation says nothing about this kind of functionality and there are no comments explaining it, I'd remove this commented code.

I'd done this in my previous changes (that have now gone into my separate branch) as there are hundreds of these lines. I'd prefer to leave them in here for now and then when a style guideline is pushed I'll put a new pr with all the commented stuff deleted

konstantin-korovin · 2025-10-20T17:51:17Z

A lot of comments but I'm not sure if they're intentional or were left in as a mistake. Not too sure of the general purpose of the pr par for adding some better output and formatting

This PR is a full branch that Neel was working (he was not making separate PRs which is not good but we have what we have). The branch should contain benchmarks that you can use (see reports I emailed) but we should make sure that merging this branch makes sense.

Th0ught09 · 2025-10-20T18:42:24Z

A lot of comments but I'm not sure if they're intentional or were left in as a mistake. Not too sure of the general purpose of the pr par for adding some better output and formatting

This PR is a full branch that Neel was working (he was not making separate PRs which is not good but we have what we have). The branch should contain benchmarks that you can use (see reports I emailed) but we should make sure that merging this branch makes sense.

Fair enough, could I have a go at cleaning it up a bit as I think the comments will lead to debt down the line?

konstantin-korovin · 2025-10-20T18:58:33Z

A lot of comments but I'm not sure if they're intentional or were left in as a mistake. Not too sure of the general purpose of the pr par for adding some better output and formatting

This PR is a full branch that Neel was working (he was not making separate PRs which is not good but we have what we have). The branch should contain benchmarks that you can use (see reports I emailed) but we should make sure that merging this branch makes sense.

Fair enough, could I have a go at cleaning it up a bit as I think the comments will lead to debt down the line?

Please go ahead!

Th0ught09 · 2025-10-23T13:46:55Z

I've done all the adjustements I think are worth the time, there's a lot more to do but I'd like to do more towards my project. The diffs are fairly large as my vim config auotmatically runs a python formatter

konstantin-korovin · 2025-10-23T15:13:32Z

@Th0ught09 thanks!

can you please run regression tests and post if there are any issues.
take a look at Neel's functions/data that you can reuse for your evaluation, also think about some simple
higher dim. examples (parameterised).

fbrausse · 2025-10-23T15:25:57Z

The diffs are fairly large as my vim config auotmatically runs a python formatter

I suggest not mixing functional changes wirh formatting changes, especially when there is no agreed-upon format, yet. Please try to keep purely syntactic changes out of PRs introducing semantic ones.

Th0ught09 · 2025-10-23T15:28:09Z

@Th0ught09 thanks!

1. can you please run regression tests and post if there are any issues.

2. take a look at Neel's functions/data that you can reuse for your evaluation, also think about some simple
   higher dim. examples (parameterised).

Forgot to mention, it runs well on my end!

Th0ught09 · 2025-10-23T15:30:49Z

The diffs are fairly large as my vim config auotmatically runs a python formatter

I suggest not mixing functional changes wirh formatting changes, especially when there is no agreed-upon format, yet. Please try to keep purely syntactic changes out of PRs introducing semantic ones.

I may not have been clear sorry, the purpose of this was formatting changes (pep8 as a ref) as a lot of the code had dangling comment lines that I thought would wind up with technical debt. The IDE I use does this automatically when I save a file so a bit hard for me to avoid it sorry!

fbrausse · 2025-10-23T17:36:58Z

The IDE I use does this automatically when I save a file so a bit hard for me to avoid it sorry!

It's pretty easy disable a plugin or auto-formatting script in vim. As you know, we've not decided on a consistent style, yet, so there's no point in everyone producing large diffs because of some random auto-formatting they've configured locally.

It will also make merges of other PRs harder, which is why those large "cleanup" changes of entire source files should be done carefully, file per file, as to not block changes from other people.

Th0ught09 · 2025-10-23T18:11:33Z

It will also make merges of other PRs harder, which is why those large "cleanup" changes of entire source files should be done carefully, file per file, as to not block changes from other people.

fair enough, I'll try rebase the mr :)

Th0ught09 · 2025-10-23T18:29:34Z

Rebased to before I touched it, I merged my previous changes into my project branch.
EDIT: Looking again it did some damage to the README, will check why it deleted so many lines
EDIT2: Not sure why it deleted so many lines but added them all back now

fbrausse · 2025-10-24T11:38:50Z

Rebased to before I touched it, I merged my previous changes into my project branch.

Many thanks!

EDIT: Looking again it did some damage to the README, will check why it deleted so many lines EDIT2: Not sure why it deleted so many lines but added them all back now

I believe, the README has, initially in this PR, been rewritten entirely to only document the changes of this PR. Ideally, there should be no lines deleted in the README. Could I ask you to take another look at it wrt. the formatting and possibly revert changes to it unless they're necessary?

Th0ught09 · 2025-10-24T15:45:08Z

Ideally, there should be no lines deleted in the README

Missed these ones sorry, all deleted lines should be restored :)

Could I ask you to take another look at it wrt. the formatting and possibly revert changes to it unless they're necessary

They aren't necessary, but I'm not sure about restoring some of the tabs that look identical, in the main page the README looks the same so would that be ok? (a bit of this may have come from manually copy pasting from the master branch)

NeelM47 and others added 21 commits July 9, 2024 17:42

Changes regarding experimentation

c2a22d3

Merge remote-tracking branch 'source/master'

ea7fa18

--

da2d858

--

b262fcb

--

6c1fe91

Plotting tool updated

703c138

Included code to monitor memory usage

2c821c2

Merge remote-tracking branch 'local_branch/bench_lowdim'

433476e

Changes to run_smlp.py

db5f340

Save changes before switching branches

bfcd35c

Updated for counter examples

a76d178

keras dropout

1f52b67

*

b818173

Merge remote-tracking branch 'local_branch/bench_lowdim' into opt

a9cc0e7

Optimisations

3945084

Merge branch 'master' into opt

6c9b469

bench_low_dim

1313113

Merge pull request #13 from NeelM47/bench_low_dim

8b31a2b

Bench low dim

Resolved merge conflict in smlp_terms.py

305cc8e

Visualization Framework

4706822

cleaning up

ec1cc2b

konstantin-korovin requested review from fbrausse and zurabk June 11, 2025 18:17

fbrausse assigned fbrausse and NeelM47 and unassigned fbrausse Jun 12, 2025

konstantin-korovin requested a review from Th0ught09 October 20, 2025 16:46

Th0ught09 requested changes Oct 20, 2025

View reviewed changes

Th0ught09 force-pushed the visualization_fw_neel_more branch from 2fbaa8d to ec1cc2b Compare October 23, 2025 18:23

Th0ught09 added 3 commits October 23, 2025 19:26

fixed merge conflict in readme

94bf256

formatting issues fixed in README

1629c91

not highlightling bold correctly

2d36512

readded removed lines from README

66268e3

fbrausse requested review from zurabksmlp and removed request for zurabk October 24, 2025 11:11

readded more deleted lines

b520e78

Conversation

konstantin-korovin commented Jun 11, 2025

Uh oh!

Th0ught09 left a comment

Choose a reason for hiding this comment

Uh oh!

Th0ught09 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

fbrausse Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Th0ught09 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

fbrausse Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Th0ught09 Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

konstantin-korovin commented Oct 20, 2025

Uh oh!

Th0ught09 commented Oct 20, 2025

Uh oh!

konstantin-korovin commented Oct 20, 2025

Uh oh!

Th0ught09 commented Oct 23, 2025

Uh oh!

konstantin-korovin commented Oct 23, 2025

Uh oh!

fbrausse commented Oct 23, 2025

Uh oh!

Th0ught09 commented Oct 23, 2025

Uh oh!

Th0ught09 commented Oct 23, 2025

Uh oh!

fbrausse commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Th0ught09 commented Oct 23, 2025

Uh oh!

Th0ught09 commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fbrausse commented Oct 24, 2025

Uh oh!

Th0ught09 commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fbrausse commented Oct 23, 2025 •

edited

Loading

Th0ught09 commented Oct 23, 2025 •

edited

Loading