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

A bug with no noise applied #76

Merged
merged 7 commits into from
Oct 30, 2023
Merged

A bug with no noise applied #76

merged 7 commits into from
Oct 30, 2023

Conversation

georgemilosh
Copy link
Contributor

In the current version of the repo the noise applied during processing is overwritten by the normalization

@GijsVermarien
Copy link
Member

@georgemilosh could you format the code with black? Then I am happy to include the changes into the repository.

@georgemilosh
Copy link
Contributor Author

Sorry what do you mean by formatting in black?

@GijsVermarien
Copy link
Member

black is the formatter we use to make sure code is always formatted with PEP8, you can do this by installing black
pip install black and then formatting the source code directory: black src/deepymod.

@georgemilosh
Copy link
Contributor Author

The thing is that I added a lot of edits to the fork meanwhile, so maybe that's why it failed?
Basically I re-run the notebooks, and added some of my material, like Allen Cahn equation etc. Tried unsuccessfully Kuramoto-Sivashnsky. Plus I found that Keller - Segel equation is no longer identified in DeePyMoD, while it was almost identified correctly in DeePyMoD_torch which also takes gradients with respect to L1 loss unlike DeePyMoD. Also to be able to run Keller - Segel equation I had to apply a small fix to the library.

@GijsVermarien
Copy link
Member

Hi George! I think it is a version thing with the black package, your edits look like great fixes and additions to me so I will merge them now.

GijsVermarien
GijsVermarien approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@GijsVermarien GijsVermarien left a comment

Choose a reason for hiding this comment

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

Many new examples are added and a few bugs were addressed. Everything looks good to me.

@georgemilosh
Copy link
Contributor Author

Concerning the examples added, a couple may need data from another equation discovery repo, which could be addressed later

@GijsVermarien GijsVermarien changed the base branch from master to georgemilosh October 30, 2023 18:13
@GijsVermarien GijsVermarien merged commit 8e68325 into PhIMaL:georgemilosh Oct 30, 2023
1 check failed
GijsVermarien added a commit that referenced this pull request Oct 30, 2023
@GijsVermarien @georgemilosh
Georgemilosh (#77)
543c2e5 
* A bug with no noise applied (#76)

* specifying the physical coefficients in the examples

* fixed a bug in Dataset class which prevented noise

* Dealing with normalization and history

* adding plotting training history functionality

* Library1D treats theta output conisistently with ODE example plus comments, stylistic choices

* adding more examples

* applying `black src/deepymod` for formatting

* Changed to a more recent version of black

* Formatted all the examples

---------

Co-authored-by: George Miloshevich <[email protected]>
@GijsVermarien
Copy link
Member

Yes, if you get any further with that feel free to open anothr PR. For now I pulled everything into the master branch!

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

@GijsVermarien GijsVermarien GijsVermarien approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@georgemilosh @GijsVermarien