Skip to content

Eparish1/opinf add neural networks 2 #96

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

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

eparish1
Copy link
Collaborator

PR for neural network branch base code. Note the re-factor of the OpInf code into its own directory, @lxmota I did this to try and keep the base code as clean as possible. If you prefer it the old way let me know.

Note, I did not add a unit test for the neural network op inf because it will not be straightforward for the runner to install all of the relevant python packages in PyCall to run the neural network opinf. Glad to talk on this more

@eparish1 eparish1 marked this pull request as draft April 30, 2025 22:16
Copy link

codecov bot commented Apr 30, 2025

Codecov Report

Attention: Patch coverage is 81.81818% with 60 lines in your changes missing coverage. Please review.

Project coverage is 91.83%. Comparing base (d46b637) to head (e0c89cc).

Files with missing lines Patch % Lines
src/opinf/opinf_ics_bcs.jl 76.65% 53 Missing ⚠️
src/solver.jl 80.95% 4 Missing ⚠️
src/opinf/opinf_model.jl 95.16% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
- Coverage   92.17%   91.83%   -0.34%     
==========================================
  Files          17       20       +3     
  Lines        3936     4117     +181     
==========================================
+ Hits         3628     3781     +153     
- Misses        308      336      +28     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lxmota lxmota marked this pull request as ready for review April 30, 2025 22:50
@lxmota
Copy link
Collaborator

lxmota commented Apr 30, 2025

@ikalash @eparish1 Is this ready for review and merge? It was in draft status but I clicked somewhere and now it says that it is ready for merge.

Yes, the tests become complex if they need Python to run. I was hoping that by moving to Julia I would avoid bringing in Matlab and Python, but then I don't know how to make sure that future changes to Norma don't break this.

@ikalash
Copy link
Collaborator

ikalash commented Apr 30, 2025

My understanding from today's meeting is Eric is still working on this but it will be ready soon. @eparish1 please correct me if I'm wrong.

@eparish1
Copy link
Collaborator Author

eparish1 commented May 1, 2025

@lxmota @ikalash It's 95% ready, I'm just running it through the CI and doing some final checks.

@eparish1
Copy link
Collaborator Author

eparish1 commented May 1, 2025

@lxmota @ikalash This is ready now. I apologize about the large PR. The large number of files changed is because I moved all of the ROM stuff to its own directory.

@lxmota, I've added a Python part to the CI to run a mock test of the NN model. The test isn't a true test because it relies on an in-development python module, but it exercises the code other than the model evaluation. If you'd rather not have python as part of the CI I can delete it. I still am not testing enough of the code to pass the codecov tests. If you'd like, I can add more tests to get this passed.

@eparish1
Copy link
Collaborator Author

eparish1 commented May 1, 2025

@lxmota Also, I thought about it more, and realized that local testing for the python-dependent things will be annoying for other users who aren't exercising this capability since it requires the right python environment. I updated the runtest.jl to only run this test if told to, and added the command to the CI.

@lxmota
Copy link
Collaborator

lxmota commented May 1, 2025

@eparish1 @ikalash I appreciate the work you have put into this, especially the effort to isolate the OpInf components and to conditionally gate the Python-based tests.

That said, I’m going to pause on merging for two reasons:

  • I’m in the middle of a fairly intrusive refactor of the time control mechanisms to enable adaptive time stepping for multi-domain simulations. This touches nearly all time-related functions, so merging this PR now would likely introduce significant conflicts and instability. Once that work lands, we’ll have a cleaner foundation for integrating additional features like this one.

  • We should also consider refactoring runtests.jl to distinguish between a default set of core tests and an optional set of extended tests, like the one for this NN feature. This would give us a cleaner way to accommodate similar cases in the future without bloating CI or introducing unnecessary friction for users.

Rant about project philosophy and external dependencies: A key motivation behind Norma.jl has been to build a self-contained, Julia-only codebase, avoiding dependencies on Matlab, Python, or other external runtimes. While I understand the appeal of leveraging PyCall and Python-based tools for neural networks, this introduces the kind of complexity (both for end users and for CI) that I have intentionally avoided. Even with the test being optional, maintaining this infrastructure going forward risks drifting away from the project's core goals.

With that in mind, I would suggest we explore moving the Python-dependent NN-based and OpInf code into a separate companion package or development branch. That would allow us to support experimentation with these advanced features while preserving a clean, Julia-native base for the mainline Norma.jl.

Lastly, I would be very open to collaborating on a Julia-native path to support neural networks and OpInf, perhaps via Flux.jl or another framework, if that becomes viable down the line.

@eparish1
Copy link
Collaborator Author

eparish1 commented May 2, 2025

@lxmota Completely understand your perspective. I'll think about things as a companion package. The main issue I see is that it needs to touch some of the core Norma code to work, so I am not sure if it can be completely orthogonal. Keeping it as a development branch is probably the most straightforward thing.

I'll learn more about some of the Julia-native NN packages and see if there is anything that can be leveraged. I don't anticipate there being an easy solution since they python packages are far more developed, but there may be things that can be utilized.

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.

3 participants