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

Add NNCG to optimizers submodule #1661

Merged
merged 29 commits into from
Nov 26, 2024
Merged

Conversation

pratikrathore8
Copy link
Contributor

No description provided.

@lululxvi
Copy link
Owner

Format the code using black https://github.com/psf/black

@pratikrathore8
Copy link
Contributor Author

I've formatted the code with black in the latest commit!

@lululxvi
Copy link
Owner

If this only supports pytorch, then it should be in https://github.com/lululxvi/deepxde/tree/master/deepxde/optimizers/pytorch

@pratikrathore8
Copy link
Contributor Author

Moved to the right folder!

@pratikrathore8
Copy link
Contributor Author

Made the formatting changes. I noticed there are other lines in the code with > 88 chars per line. Would you like for me to fix those?

@lululxvi
Copy link
Owner

lululxvi commented Mar 3, 2024

Made the formatting changes. I noticed there are other lines in the code with > 88 chars per line. Would you like for me to fix those?

Yes.

Could you also fix the Codacy issues?

@pratikrathore8
Copy link
Contributor Author

pratikrathore8 commented Mar 4, 2024

The formatting has been fixed.

There are still some Codacy issues, but these are mainly due to "too many local variables" and the __init__ function in the optimizer, which is just following PyTorch conventions.

What do we need do next to further integrate NNCG into the codebase?

@lululxvi
Copy link
Owner

lululxvi commented Mar 5, 2024

@pratikrathore8
Copy link
Contributor Author

I've added NNCG to optimizers.py and NNCG_options to config.py

@pratikrathore8
Copy link
Contributor Author

Fixed!

@pratikrathore8
Copy link
Contributor Author

Fixed!

@lululxvi
Copy link
Owner

Have you tried using NNCG in some demo examples?

@pratikrathore8
Copy link
Contributor Author

A quick update:

I've started thinking about how to make a demo example that uses NNCG. The plan would be to modify some of the existing examples (e.g. Burgers.py) and add on NNCG after Adam and L-BFGS.

I'll have to make some modifications to model.py to full integrate NNCG into the training pipeline. I'm aiming to have an example fully working by the end of next week!

@pratikrathore8
Copy link
Contributor Author

@lululxvi Made the changes. Should we consider merging _train_pytorch_nncg into _train_sgd by adding an argument to _train_sgd that decides whether or not to perform a backward pass on the loss? Or would you prefer we leave the functions separate, despite the code duplication?

deepxde/model.py Outdated Show resolved Hide resolved
@pratikrathore8
Copy link
Contributor Author

@lululxvi I added a train_step_nncg function as you suggested. NNCG now runs using _train_sgd.

Thanks for your patience with the refactoring steps so far!

@pratikrathore8
Copy link
Contributor Author

@lululxvi Done!

@lululxvi
Copy link
Owner

lululxvi commented Nov 5, 2024

Add a document for the example similar to this https://deepxde.readthedocs.io/en/latest/demos/pinn_forward/burgers.html

@pratikrathore8
Copy link
Contributor Author

pratikrathore8 commented Nov 5, 2024

@lululxvi I added the demo. It's basically the same as the one you linked, except I added the part with NNCG to the end.

Is there a way to check if it's been properly integrated into the docs?

@lululxvi
Copy link
Owner

lululxvi commented Nov 7, 2024

Please resolve the conflict.

You can look at the generated doc by clicking "Details" at the right side of "docs/readthedocs.org:deepxde" in in the check list.

@pratikrathore8
Copy link
Contributor Author

@lululxvi I've fixed the merge conflict!

I took a look at the doc for the demo, and it seems good to me.

@pratikrathore8
Copy link
Contributor Author

These are all great suggestions! I've incorporated them into my most recent commit.

@pratikrathore8
Copy link
Contributor Author

Good catch! I made the fix in my most recent commit.

@lululxvi lululxvi merged commit 8275aeb into lululxvi:master Nov 26, 2024
10 of 11 checks passed
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.

2 participants