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

Make ompy more modular #195

Open
vetlewi opened this issue Oct 28, 2021 · 3 comments
Open

Make ompy more modular #195

vetlewi opened this issue Oct 28, 2021 · 3 comments
Labels
Suggestion Suggestion for new feature/changes
Milestone

Comments

@vetlewi
Copy link
Collaborator

vetlewi commented Oct 28, 2021

At some point we should consider separating the physics and the "core" classes into different modules. One way could be to have an ompy.core module that contains the Matrix, Vector classes and misc. library functions that are strictly not Oslo method related.

@vetlewi vetlewi added the Suggestion Suggestion for new feature/changes label Oct 28, 2021
@vetlewi vetlewi added this to the Version 2.0 milestone Oct 28, 2021
@ErlendLima
Copy link
Contributor

What would be the benefit? I agree with the goal of modularity, but due to how inflexible Python is (e.g a class can only have one definition, each method can only have one definition), and how many moving parts the Oslo method has, decoupling the Matrix and Vector classes from the physics code is impractical. It is possible to write Matrix and Vector to be more OMpy-agnostic, but this would lead to OMpy-code becoming more verbose and difficult to understand.

For example, the plotting of Matrix.plot() only makes sense in the context of Oslo method matrices. Decoupling the plotting from the class (which, in my mind, makes much more Platonic sense. A matrix is a bag of numbers, not a plotting tool) would require the plotting to be rewritten to, say, ompy.plot.plot_raw(matrix), which is less idiomatic Python. In the end, nothing is gained, since matrices are very rarely plotted any other way.

Another example would be the index arrays, such as Matrix.Eg. The names only makes sense when working with Eg-Ex matrices. To make them agnostic of OMpy, they would need a more general name, like Matrix.X or Matrix.E1. In the first versions of OMpy, that was their naming, but this rendered the code that used them difficult to read.

In short, I agree with the intent, but it would introduce more problems than it solves. This is due to the limitations of Python more than OMpy. I have been writing an OMpy clone in Julia, where this problem does not exist, as the implementation of a type and its methods are distinct.

I'm very much open to a better OMpy design, if you have any ideas:)

@vetlewi
Copy link
Collaborator Author

vetlewi commented Nov 4, 2021

What would be the benefit? I agree with the goal of modularity, but due to how inflexible Python is (e.g a class can only have one definition, each method can only have one definition), and how many moving parts the Oslo method has, decoupling the Matrix and Vector classes from the physics code is impractical. It is possible to write Matrix and Vector to be more OMpy-agnostic, but this would lead to OMpy-code becoming more verbose and difficult to understand.

For example, the plotting of Matrix.plot() only makes sense in the context of Oslo method matrices. Decoupling the plotting from the class (which, in my mind, makes much more Platonic sense. A matrix is a bag of numbers, not a plotting tool) would require the plotting to be rewritten to, say, ompy.plot.plot_raw(matrix), which is less idiomatic Python. In the end, nothing is gained, since matrices are very rarely plotted any other way.

Another example would be the index arrays, such as Matrix.Eg. The names only makes sense when working with Eg-Ex matrices. To make them agnostic of OMpy, they would need a more general name, like Matrix.X or Matrix.E1. In the first versions of OMpy, that was their naming, but this rendered the code that used them difficult to read.

In short, I agree with the intent, but it would introduce more problems than it solves. This is due to the limitations of Python more than OMpy. I have been writing an OMpy clone in Julia, where this problem does not exist, as the implementation of a type and its methods are distinct.

I'm very much open to a better OMpy design, if you have any ideas:)

I might not have made my idea clear enough😅 I don't suggest a major change to the design. The point was to make it possible to install OMpy without having to install dependencies such as Multinest. There are lots of useful functionality for manipulating, visualizing and importing/exporting spectra and matrices that doesn't need to rely on external packages.

The way one could achieve this would be to re-organize the package structure to something like:

ompy/
├── core/
│    ├── matrix.py
│    ├── vector.py
│    └── etc.
├── method/
│    ├── unfolder.py
│    ├── first_generation.py
│    └── etc.
├── normalizing/     <---- Requires Multinest
│    ├── normalize_nld.py
│    ├── normalize_gsf.py
│    └── etc.
└── __init__.py

where one can install just the "core" functionality, pip install ompy-core or the entire package pip install ompy.

@ErlendLima
Copy link
Contributor

Ah, I understand! I like the idea:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Suggestion Suggestion for new feature/changes
Projects
None yet
Development

No branches or pull requests

2 participants