-
Notifications
You must be signed in to change notification settings - Fork 2
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
Refactoring and improvement of msm covariance matrices #31
Conversation
black_it/loss_functions/msm.py
Outdated
raise ValueError( | ||
"please specify a valid covariance matrix. Supported options are: 'identity', 'inverse_variance', " | ||
"or directly a numpy array." | ||
) |
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.
at this point it is probably missing a check on the type of covariance_mat
? Below we assume that if covariance_mat
is not a str
, then it is a NDArray
, but this is not enforced anywhere. On one hand, this follows the Python philosophy "we are all consenting adults" and the user is responsible of what he's doing. Perhaps a one-more check is not a bad idea though.
) | |
) | |
if not isinstance(covariance_mat, np.ndarray): | |
raise ValueError(...) |
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.
thank you, I added this check
black_it/loss_functions/msm.py
Outdated
1.0 / np.mean((real_mom_1d[None, :] - ensemble_sim_mom_1d) ** 2, axis=0) | ||
) | ||
else: | ||
cast(self._covariance_mat, NDArray[np.float64]) |
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.
this is ok if the check in the constructor is added. It is slightly-less OK otherwise, but still good enough :)
ab2696b
to
8b60174
Compare
black_it/loss_functions/msm.py
Outdated
# if we were given no covariance matrix, then we'll use a default | ||
# one, and we can't do any further validation (without executing the | ||
# moment_calculator) | ||
if covariance_mat in ("identity", "inverse_variance"): |
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.
Possible improvement. Instead of this, we could have an Enum
class:
class CovarianceMatrixType:
IDENTITY = "identity"
INVERSE_VARIANCE = "inverse_variance"
And then here (we should first check it is not a string!):
try:
covariance_mat = CovarianceMatrixType(covariance_mat)
except ValueError:
raise ValueError(f"expected one of {list(map(lambda x: x.value, CovarianceMatrixType))}, got {covariance_mat})
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.
Ok thanks, I implemented this too
8b60174
to
2f63037
Compare
…ise_moments'' f f f f
2f63037
to
80c7805
Compare
Co-authored-by: Marco Favorito <[email protected]> f
7a74765
to
26396ca
Compare
Proposed changes
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply.develop
branch (left side). Also you should start your branch off ourdevelop
.