-
Notifications
You must be signed in to change notification settings - Fork 112
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
ML keras #4172
base: master
Are you sure you want to change the base?
ML keras #4172
Conversation
jenkins build this please |
jenkins build this please |
This does not add a dependency on a third party library, so then I assume it instead embeds Keras in some way? Or have I misunderstood the purpose of this PR? |
Is there a reason this is added to opm-common instead of being a separate repository? Do we, somehow, need to make (selected) objects in this repository, or any of its downstream repositories for that matter, "aware" of Keras? |
We use Keras for the training process (it doesnt need to be done in OPM). The generated models are subsequently embedded and run in OPM. I have updated the description to provide some context. |
jenkins build this please |
jenkins build this please |
Maybe I'm missing something, but as far as I can tell no-one have answered my question from last week
I would really like an answer to this before I consider the details of the PR. |
Sorry for not answering earlier. The idea is to apply the ML-Keras inside OPM for different tasks. This is only the first PR to add the Keras ML model. The applications will follow. For an example of a ML near well model using ML-Keras check out https://github.com/cssr-tools/ML_near_well. Since the ML-Keras model framework is general. We hope it would be useful for the OPM community and therefore suggest to add it to opm-common |
Okay, utility/convenience is clearly one reason for adding it here. Would it be impossible to make [your/certain use cases] work if it were located elsewhere? Do you, for instance, need access to the internals/private data members or member functions of |
jenkins build this please |
Exact! For instance, we need access to the automatic differentiation tools within OPM. |
jenkins build this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of changes needed here. I have only looked at the C++ code, and I probably missed some. I have not really checked that the activation functions or the layers do what a user of Keras would expect. I have not looked at any of the Python code, someone else must do that.
I have requested many changes, but I hope it provides a useful learning experience. Feel free to ask about anything that is unclear!
opm/ml/ml_tools/__init__.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to add this empty file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.:
The
__init__.py
files are required to make Python treat directories containing the file as packages (unless using a namespace package, a relatively advanced feature). This prevents directories with a common name, such as string, from unintentionally hiding valid modules that occur later on the module search path. In the simplest case,__init__.py
can just be an empty file, but it can also execute initialization code for the package or set the__all__
variable, described later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While an empty file is fine, you might want to export some functions here, primarily the function export_model
.
Copyright (c) 2024 NORCE | ||
This file is part of the Open Porous Media project (OPM). | ||
|
||
OPM is free software: you can redistribute it and/or modify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the MIT license is compatible with GPLv3, it is fine to include this and use it with OPM Flow. But I would like to clarify: what is the license of this file? Is it MIT or GPL?
If this file is mostly the work of Rose and Maevskikh, then it may be fair to make the entire file MIT licensed. However it is clearly allowed to license it under GPLv3 instead, especially if the NORCE contribution here is substantial.
|
||
|
||
template<class Evaluation> | ||
bool KerasModel<Evaluation>::LoadModel(const std::string& filename) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this file format is defined by Keras? Please include in a comment a reference.
KASSERT(layers_[i]->Apply(&temp_in, &temp_out), | ||
"Failed to apply layer %d", i); | ||
|
||
temp_in = temp_out; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement an O(1) swap() function instead, and avoid the copying in the layer apply() functions.
KASSERT(out, "Invalid output"); | ||
KASSERT(in->dims_.size() <= 2, "Invalid input dimensions"); | ||
|
||
if (in->dims_.size() == 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dimension things here require an explanation. Why can we not apply a dense layer to a 1-tensor, and why is there a special treatment for 2-tensors here?
|
||
bool result = layer->LoadLayer(&file); | ||
if (!result) { | ||
printf("Failed to load layer %d", i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to replace lots of printing with proper logging. Using OpmLog::error() for things like this, and fmt::format() to do the formatting is what is done elsewhere, and should be done here as well.
For now though, you should concentrate on the other changes requested before you do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments on the python bits. I think a major point is that the folder opm/ml_tools
, which contains only python code, should probably be moved to say the python
folder or similar. And since these scripts use external libraries (tf, numpy, keras) I would really like to see a requirements.txt
file specifying the versions used. Especially tensorflow is known to be problematic her.
# * Copyright (c) 2018 Paul Maevskikh | ||
# * | ||
# * MIT License, see LICENSE.MIT file. | ||
# */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the C style comment here? A normal python style comment would suffice. The copyright statement should probably be on one line, together with the NORCE statement. (and consider Atgeirr's comment about license compatibility)
else: | ||
assert False, "Unsupported activation type: %s" % activation | ||
|
||
model_layers = [l for l in model.layers if type(l).__name__ not in ['Dropout']] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the indirection through type(l).__name__
here? Wouldn't isinstance(l, Dropout)
be clearer?
elif activation == 'hard_sigmoid': | ||
f.write(struct.pack('I', ACTIVATION_HARD_SIGMOID)) | ||
else: | ||
assert False, "Unsupported activation type: %s" % activation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else: | ||
assert False, "Unsupported activation type: %s" % activation | ||
|
||
model_layers = [l for l in model.layers if type(l).__name__ not in ['Dropout']] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again probably easier to have isinstance(l, Dropout)
write_activation(activation) | ||
|
||
else: | ||
assert False, "Unsupported layer type: %s" % layer_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opm/ml/ml_tools/__init__.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While an empty file is fine, you might want to export some functions here, primarily the function export_model
.
import numpy as np | ||
import tensorflow as tf | ||
from numpy.typing import ArrayLike | ||
from tensorflow import keras |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using external libraries I think it would be a good idea to supply a requirements.txt file to fix versions, so that a future user will be able to run these scripts.
Thanks all for the valuables comments and suggestions. |
Integrating Keras capabilities to OPM.
Draft pull request to test/discuss implications of the changes in OPM.
This enables the straightforward and adaptable integration of neural networks into OPM scripts. These models are initially trained using the Keras library in Python, stored in a format readable for the OPM framework and subsequently deployed within OPM.
When the user initializes and loads a stored Keras model inside an OPM script, an automated deployment process handles all the translation. This process works by operating a series of steps handling model interpretation, layer conversion, optimization, and code generation steps to adapt the Keras model to a native OPM function.