Adding a simple HSC Autoencoder as an example #136

aritraghsh09 · 2024-12-08T15:30:24Z

Change Description

My PR includes a link to the issue that I am addressing

This PR does the following:-

Adds a very simple autoencoder that works with the HSC data and has a training model so generic that users should be able to use it widely for a large number of DL models
Adds instructions to the training demo on how to run tensorboard when running it remotely on a server

This is currently marked as a draft because of a an issue in the change I implemented in fibad_default_config.toml (see below)

…C Data

…adam

review-notebook-app · 2024-12-08T15:30:29Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2024-12-08T15:33:10Z

Codecov Report

Attention: Patch coverage is 34.61538% with 17 lines in your changes missing coverage. Please review.

Project coverage is 39.64%. Comparing base (cc2f12b) to head (1b6a76e).
Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
src/fibad/models/example_hsc_autoencoder.py	30.43%	16 Missing ⚠️
src/fibad/models/model_registry.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #136      +/-   ##
==========================================
- Coverage   39.72%   39.64%   -0.09%     
==========================================
  Files          22       23       +1     
  Lines        1815     1839      +24     
==========================================
+ Hits          721      729       +8     
- Misses       1094     1110      +16

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

github-actions · 2024-12-08T15:33:36Z

Before [`cc2f12b`]	After [`0827ac6`]	Ratio	Benchmark (Parameter)
2.36±0.8s	953±800ms	~0.40	benchmarks.time_computation
2.76k	1.36k	0.49	benchmarks.mem_list

Click here to view all benchmarks.

aritraghsh09 · 2024-12-08T15:39:27Z

src/fibad/fibad_default_config.toml

+
+# Default PyTorch optimizer parameters. The keys match the names of the parameters
+lr = 0.01
+#momentum = 0.9


@drewoldag @mtauraso

I am using optim.Adam as my optimizer, which doesn't take momentum as an argument. So, the only way to make it work in the current implementation was to set dev_mode = true in my local config file and to comment out momentum (above) in the default configuration file.

I tried setting momentum=false in my local config, but that didn't take care of the above. What do you think is a better solution here? As all optimizers might/mightn't take different arguments.

This also raises the question of the default behavior of our configuration system to assume defaults via the default config file and then come up with a combination using the default and user-specified configs? For me, personally, this has been confusing sometimes. Can't we ask users to ask users to copy the default config file and modify it as needed? And then only that file is used for reading the configs?

These are both good questions, and it can drive a good conversation during the sync meeting today.

For the first question, regarding different arguments for different optimizers and loss functions, one thing we could potentially do is introspect the function that is about to be called (in this case optim.Adam) and get the list of parameters that it can accept, then only pass the config values that match one of those. That might be the most transparent to the user in general.

Another approach would be to create a subtable in the config file for the optimizer of choice, but this feels unnecessarily burdensome. Something like:

[optimizer] name = 'torch.optim.Adam' ... [optimizer.Adam] lr = 0.01 < other Adam params > [optimzer.Other] < params that are only for "Other" >

Regarding the second question about just making a copy of the config and using that. In the short term we could implement that, but we've built the config system to be able to support external model/dataset libraries as well as those that are built in.

For a user working with an external library, we would want the defaults of that library to override the fibad defaults, and then the user specific values to override anything else. I think that for a new user, that the first step of merging fibad and external defaults becomes a bit challenging (or at least annoying)

For the suggested workaround of momentum=False you can make the following change and it should work:

47c4221

If you git fetch you should be able to git cherry-pick 47c42213470be3e14c9b7e569d41bde94b9337a8 in your PR branch to pull in the change.

For this PR I'd advocate cherry-picking this change, and putting the rest of the config refactor discussed above and in the meeting into an issue to be completed separately from this PR.

mtauraso · 2024-12-11T23:05:01Z

docs/notebooks/train_model.ipynb

-    "%tensorboard --logdir ./results"
+    "%tensorboard --logdir ./results\n",
+    "\n",
+    "# if running on a remote server, and tunnelling a connection,\n",


It seems like there is a tensorboard integration feature buried in this comment :)

We should probably flesh it out a bit.

mtauraso · 2024-12-11T23:07:20Z

src/fibad/models/model_registry.py

+    del arguments["name"]
+    return optimizer_cls(self.parameters(), **arguments)
+
+


You may want to rebase this on main before merging, this looks like some appearance of one of Drew's earlier commits from PR #132 GIt should sort this out in the rebase and give you a clean diff here.

mtauraso

Some minor comments but nothing big. This should get merged even if there is disagreement on my commentary.

During installation, we only send error messages to stdout. Specifying this now in the messages that are printed during installation.

Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 4 to 5. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@v4...v5) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [dawidd6/action-download-artifact](https://github.com/dawidd6/action-download-artifact) from 6 to 7. - [Release notes](https://github.com/dawidd6/action-download-artifact/releases) - [Commits](dawidd6/action-download-artifact@v6...v7) --- updated-dependencies: - dependency-name: dawidd6/action-download-artifact dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…nloaded files (#130) - Should fix issue #127. - Moved removal of incomplete downloads from the prune stage to the f/s read stage - Added a better error to the case where HSCDataSet arrives at an absurdly small size of image to crop to. Co-authored-by: Drew Oldag <[email protected]>

…se the default

aritraghsh09 · 2024-12-12T19:11:49Z

I cherry picked 47c42213470be3e14c9b7e569d41bde94b9337a8 but had to modify the logic slightly to make it right. I will be merging this soon now.

drewoldag and others added 6 commits December 6, 2024 16:30

Initial commit allowing definition of Loss and Optimizer in config file.

4d6e778

Initial commit of HSC Autoencoder

39125e3

Initial implementation of a simple autoencoder that works with the HS…

6e79482

…C Data

Example Autoencoeder that works on HSC 3,262,262 images

47fff10

Temporarily Commenting out momentum as it is not a valid keyword for …

6d40e70

…adam

Adding instructions for users running tensorboard on a server

7b6ef6c

aritraghsh09 commented Dec 8, 2024

View reviewed changes

aritraghsh09 requested review from drewoldag and mtauraso December 8, 2024 15:39

mtauraso reviewed Dec 11, 2024

View reviewed changes

mtauraso approved these changes Dec 11, 2024

View reviewed changes

aritraghsh09 and others added 7 commits December 12, 2024 06:17

Making install messages more verbose

0cc1896

During installation, we only send error messages to stdout. Specifying this now in the messages that are printed during installation.

Suggested workaround to allow momentum=False in a user config to exci…

167e8da

…se the default

Update to arugments dictionary within optimizer and other comments

ccf0ca7

Merge branch 'main' into hsc_autoencoder

1b6a76e

aritraghsh09 marked this pull request as ready for review December 12, 2024 19:10

aritraghsh09 merged commit 4d60235 into main Dec 12, 2024
6 of 8 checks passed

aritraghsh09 deleted the hsc_autoencoder branch December 12, 2024 19:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a simple HSC Autoencoder as an example #136

Adding a simple HSC Autoencoder as an example #136

aritraghsh09 commented Dec 8, 2024 •

edited

Loading

review-notebook-app bot commented Dec 8, 2024

codecov bot commented Dec 8, 2024 •

edited

Loading

github-actions bot commented Dec 8, 2024 •

edited

Loading

aritraghsh09 Dec 8, 2024 •

edited

Loading

drewoldag Dec 9, 2024

mtauraso Dec 11, 2024 •

edited

Loading

mtauraso Dec 11, 2024

mtauraso Dec 11, 2024 •

edited

Loading

mtauraso left a comment

aritraghsh09 commented Dec 12, 2024

		del arguments["name"]
		return optimizer_cls(self.parameters(), **arguments)

Adding a simple HSC Autoencoder as an example #136

Adding a simple HSC Autoencoder as an example #136

Conversation

aritraghsh09 commented Dec 8, 2024 • edited Loading

Change Description

review-notebook-app bot commented Dec 8, 2024

codecov bot commented Dec 8, 2024 • edited Loading

Codecov Report

github-actions bot commented Dec 8, 2024 • edited Loading

aritraghsh09 Dec 8, 2024 • edited Loading

Choose a reason for hiding this comment

drewoldag Dec 9, 2024

Choose a reason for hiding this comment

mtauraso Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

mtauraso Dec 11, 2024

Choose a reason for hiding this comment

mtauraso Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

mtauraso left a comment

Choose a reason for hiding this comment

aritraghsh09 commented Dec 12, 2024

aritraghsh09 commented Dec 8, 2024 •

edited

Loading

codecov bot commented Dec 8, 2024 •

edited

Loading

github-actions bot commented Dec 8, 2024 •

edited

Loading

aritraghsh09 Dec 8, 2024 •

edited

Loading

mtauraso Dec 11, 2024 •

edited

Loading

mtauraso Dec 11, 2024 •

edited

Loading