Add NMR data model, integration with EnzymeML, and peak assignment features by torogi94 · Pull Request #13 · NMRPy/nmrpy

torogi94 · 2025-01-20T15:13:43Z

Add md-models-based research data model for NMR
- Collect NMR parameters from instrument metadata
- Record processing steps preformed in NMRpy
- Save both the raw data and the latest version of the processed data
- Assign species from a list or from an EnzymeML document to the peaks
Add EnzymeML integration using pyenzyme library
- Load EnzymeML document into a FidArray
- Have EnzymeML species available within Fid object
- Save calculated concentrations to and return the EnzymeML document
Add widgets to assign peaks either on the entire FidArray or single Fid objects
- Use EnzymeML species if document available
- Pass list of EnzymeML species if no document has been loaded yet
- Use list of simple strings instead of working with EnzymeML standard
- Use only a slice of Fids when assigning on the entire FidArray by passing an index list

jmrohwer · 2025-01-20T19:56:48Z

@torogi94 Thanks for this!
I need some time to look through this. But from a brief inspection the following come to mind:

Dependencies: utils.py imports sympy as well as pyenzyme, yet none of them are declared in requirements.txt. We should discuss dependencies and which of these are needed for core functionality. One option is to have optional dependencies for certain functionalities. Pyenzyme itself has a multitude of dependencies. I am not keen for the core NMRPy library to have a huge list of dependences. Associated with that the version requirement dependency hell.
Related: how stable are pyenzyme, md-models and the other dependencies from your group? E.g. Pyenzyme vs. Pyenzyme2? My concern is long-term maintenance.
The merge conflict needs to be resolved.
Looking at data_objects.py as well as plottling.py there are a very large number of changes relating to linting and formatting (e.g. single vs. double quotes, linebreaks etc.). Are these linting changes done in a separate commit? As it is, it's very tedious to separate the real code changes from "cosmetic" linting.

torogi94 · 2025-01-23T15:48:59Z

Thank you for looking over it! Here, a few comments:

Dependencies: utils.py imports sympy as well as pyenzyme, yet none of them are declared in requirements.txt. We should discuss dependencies and which of these are needed for core functionality. One option is to have optional dependencies for certain functionalities. Pyenzyme itself has a multitude of dependencies. I am not keen for the core NMRPy library to have a huge list of dependences. Associated with that the version requirement dependency hell.

Good catch! I really did forget to declare the new dependencies in the requirements.txt file. I think you are right that we should not necessarily introduce them to the core of NMRpy. How about we declare them as optional nmrpy[enzymeml]? For the NMRpy data model, however, it would be easier if we could make pydantic a core dependency. If I am not mistaken, it is the only one required for the data model to work. If not, I can restructure my code, so the data model part becomes entirely optional, too, though.

Related: how stable are pyenzyme, md-models and the other dependencies from your group? E.g. Pyenzyme vs. Pyenzyme2? My concern is long-term maintenance.

We are preparing a new stable pyenzyme (v2) release which we can then pin in the requirements. In my experience, it is already running better and will be more maintainable than the previous v1 version. With regard to md-models, the NMRpy data model is not dependent at all on the md-models version. Actually, it is not even a dependency needed, as all functionalities are handled via pydantic, which is itself a very well maintained library!

The merge conflict needs to be resolved.

Absolutely. I’ll merge main into my branch and fix the conflict manually so you can have a clean merge without conflicts!

Looking at data_objects.py as well as plottling.py there are a very large number of changes relating to linting and formatting (e.g. single vs. double quotes, linebreaks etc.). Are these linting changes done in a separate commit? As it is, it's very tedious to separate the real code changes from "cosmetic" linting.

Sorry for the noisy diff, it seems I accidentally let my linter loose on the two files and did not even notice... I’ll revert the linting changes from this PR and submit them in a separate PR (like discussed).

jmrohwer · 2025-01-24T07:34:00Z

Good catch! I really did forget to declare the new dependencies in the requirements.txt file. I think you are right that we should not necessarily introduce them to the core of NMRpy. How about we declare them as optional nmrpy[enzymeml]? For the NMRpy data model, however, it would be easier if we could make pydantic a core dependency. If I am not mistaken, it is the only one required for the data model to work. If not, I can restructure my code, so the data model part becomes entirely optional, too, though.

I agree with making pydantic a core dependency and the others as optional in nmrpy[enzymeml].

On another note. I also have a conda build recipe as well as a CI job that builds and distributes a conda package. This has been very easy to date because NMRPy is a pure python package and there are/were conda packages available for all of the dependencies (channel conda-forge). Is PyEnzyme released as a conda package? I would like to continue distributing conda packages, just as we do for PySCeS as well.

We are preparing a new stable pyenzyme (v2) release which we can then pin in the requirements. In my experience, it is already running better and will be more maintainable than the previous v1 version. With regard to md-models, the NMRpy data model is not dependent at all on the md-models version. Actually, it is not even a dependency needed, as all functionalities are handled via pydantic, which is itself a very well maintained library!

Okay great.

Sorry for the noisy diff, it seems I accidentally let my linter loose on the two files and did not even notice... I’ll revert the linting changes from this PR and submit them in a separate PR (like discussed).

Give me a ping once you are ready with all of this then I'll proceed with the detailed code review 😄

torogi94 · 2025-01-29T09:46:32Z

I think I have addressed all points mentioned so far, so you may continue with the detailed code review now!

On another note. I also have a conda build recipe as well as a CI job that builds and distributes a conda package. This has been very easy to date because NMRPy is a pure python package and there are/were conda packages available for all of the dependencies (channel conda-forge). Is PyEnzyme released as a conda package? I would like to continue distributing conda packages, just as we do for PySCeS as well.

I am in contact with @JR-1991 about the conda-forge release. While it was not planned initially, he says we can make it happen! I will come back to you about this when we are preparing the PyPI release of v2. For now, the GitHub version is still used in the new optional requirements for nmrpy[enzymeml]. Of course, I will change that to the stable PyPI version as soon as it is available!

torogi94 · 2025-05-05T15:21:49Z

I have addressed a few more issues we discussed in pull requests #14 and #15 and merged them with this pull request:

With Add measurement handling #14, handling of multiple EnzymeML measurements is now possible.
With Rework data handling #15, data handling within NMRpy has been optimized to keep memory requirements and computation times low.

torogi94 · 2025-08-27T14:01:12Z

@jmrohwer: In #18, I have added handling of t0 values in MeasurementData of EnzymeML documents to the library. Initial values can now be set using FidArray.add_t0_to_enzymeml(). Both an option for interactive assignment using the new T0Adder widget and script-like handling when setting gui=False are available. The user has the option to either using the existing t1 values as t0, if no initials have been measured, or provide measured t0 values instead. Furthermore, an offset value to account for delays in measurements can be applied to the time array.

- Initialisation of enzymeml_document and concentrations properties of FidArray class with initial None value led to Error. Check for None added to fix this issue. - Add correct optional dependency name to setup.py

- Move the setup of Peak objects from the Peak(Range)Assigner to the deconvolution methods to prevent uninitialised Peak objects in data model. - Update create_enzymeml() method to reflect changes in pyenzyme library. - Add species property to FidArray, similar to the deconvoluted_integrals property.

Due to an issue with md-models, pydantic>=2.10.0 is currently causing errors. In the meantime, a version restraint has been added to the requirements.txt

Fix type checking bug in enzymeml_document property of FidArray class.

* Add create_new_enzymeml_measurement() method - FidArray now has a create_new_enzymeml_measurement() method that acts as the interface for creating new Measurement objects either per GUI or script. - FidArray enzymeml_document property now checks for existence of at least one Measurement object in adder method. - plotting.py now has a skeleton MeasurementCreator class that will later be the GUI for creating new Measurement objects. - utils.py now has a utility function that creates an EnzymeML Measurement object from the parameters passed to it. * Add multiple Measurement support - Add MeasurementCreator widget as GUI for create_new_enzymeml_measurement() method - Update apply_to_enzymeml() method - Update create_enzymeml() method

* Update EnzymeML species handling - Add species type flags to get_species_from_enzymeml() util function to allow filtering for specific types of species. - Remove unnecessary display of proteins in PeakAssigner and PeakRangeAssigner using the new species type flags. * Add flags for keeping data models upon saving * Change data array handling of NMRpy data model - Add save_data_model() method to serialise the NMRpy data model. - Change handling of data arrays: They are now saved as numpy.ndarrays in each Fid object and only copied as lists into the data model upon serialisation. * Update data_objects.py - Resolve Pydantic serialisation issues (complex → string conversion). - Optimise processing loops for faster execution and lower memory usage. * Change keep_data_model flag to False

… save, not set to None

- Added T0Tab and T0Logic classes to utils.py - Added T0Adder widget class to plotting.py - Added add_t0_to_enzymeml() method that can be used interactively with a Jupyter widget using gui=True or script-like with gui=False

Changed behaviour of Fid._setup_peak_objects(). Multiple peaks per range are handled properly now by using Fid._grouped_peaklist instead of Fid.peaks. Also refactored the method to make it more robust and readable overall.

- Added NMR parameters to fid_object setter - Fixed p0 and p1 phasing parameter assignment

Copilot

Pull request overview

This PR introduces an NMR-focused research data model aligned with md-models/EnzymeML, adds EnzymeML integration (via optional pyenzyme), and extends interactive widgets for peak assignment and measurement editing.

Changes:

Added NMRpy/EnzymeML-aligned data model specifications and an auto-generated Pydantic schema (JSON-LD capable).
Implemented EnzymeML helpers (species lookup, measurement construction, applying concentrations) and new peak/t0/measurement widgets.
Updated packaging/tooling (optional extras for EnzymeML, dependency bumps, Ruff config) and added/extended tests.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 18 comments.

Show a summary per file

File	Description
specifications/nmrpy.md	Adds written spec for the NMRpy data model objects/enums.
specifications/datamodel_schema.md	Adds Mermaid class diagram for the data model structure.
setup.py	Adds EnzymeML optional extras and updates author metadata.
ruff.toml	Introduces Ruff formatting/lint configuration.
requirements.txt	Updates baseline dependencies and adds pydantic.
nmrpy/utils.py	Adds EnzymeML utility helpers (species, measurements, serialization helpers).
nmrpy/tests/nmrpy_tests.py	Adds tests for new data model + EnzymeML-related setters.
nmrpy/plotting.py	Adds peak assignment + t0/measurement widgets and integrates EnzymeML utilities.
nmrpy/nmrpy_model.py	Adds generated Pydantic models for the NMRpy data model (JSON-LD fields).
nmrpy/data_objects.py	Integrates data model + EnzymeML into Fid/FidArray and processing steps tracking.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-17T15:49:47Z

nmrpy/plotting.py

+from typing import Mapping, Optional
+
+from pyenzyme.versions.v2 import Protein


This unconditional pyenzyme import will raise at import-time when pyenzyme isn't installed, even though the rest of the file treats it as an optional dependency. Move Protein into the existing try: block (or avoid importing it directly and use getattr(pyenzyme, ...) / isinstance(..., pyenzyme.Protein) inside the guarded section).

Copilot · 2026-02-17T15:49:47Z

nmrpy/data_objects.py

+            self.__fid_object.raw_data = [str(datum) for datum in self.raw_data]
+            self.__fid_object.processed_data = [float(datum) for datum in self.data]
+        except AttributeError:


float(datum) will raise TypeError when self.data contains complex values (common before .real()), but only AttributeError is caught here. Update serialization logic to handle complex processed data (e.g., store strings for complex values) and/or catch TypeError to avoid hard crashes when accessing fid_object.

Suggested change

self.__fid_object.raw_data = [str(datum) for datum in self.raw_data]

self.__fid_object.processed_data = [float(datum) for datum in self.data]

except AttributeError:

# Always serialise raw_data as strings

self.__fid_object.raw_data = [str(datum) for datum in self.raw_data]

# Serialise processed_data, handling complex or otherwise non-floatable values gracefully

processed_data = []

for datum in self.data:

try:

processed_data.append(float(datum))

except (TypeError, ValueError):

# Fallback for complex or non-scalar values: store string representation

processed_data.append(str(datum))

self.__fid_object.processed_data = processed_data

except (AttributeError, TypeError):

Copilot · 2026-02-17T15:49:48Z

nmrpy/data_objects.py

                    )
            mz = lmfit.minimize(Fid._phased_data_sum, p, args=([data]), method=method)
-            phased_data = Fid._ps(data, p0=mz.params['p0'].value, p1=mz.params['p1'].value)
+            phased_data, p0, p1 = Fid._ps(data, p0=mz.params['p0'].value, p1=mz.params['p1'].value)


_ps() converts p0/p1 from degrees to radians, and now returns the radian values, which are then propagated to callers and stored in the data model. This changes external behavior (previously phase angles were effectively in degrees everywhere). Keep returning degrees (e.g., return the original degree inputs) or return both but ensure the values stored/exposed match the documented units (the UI output currently says degrees).

Copilot · 2026-02-17T15:49:48Z

nmrpy/data_objects.py

            if verbose:
                print('Zero order: %d\tFirst order: %d\t (In degrees)'%(mz.params['p0'].value, mz.params['p1'].value))
-            return phased_data
+            return phased_data, p0, p1


_ps() converts p0/p1 from degrees to radians, and now returns the radian values, which are then propagated to callers and stored in the data model. This changes external behavior (previously phase angles were effectively in degrees everywhere). Keep returning degrees (e.g., return the original degree inputs) or return both but ensure the values stored/exposed match the documented units (the UI output currently says degrees).

Copilot · 2026-02-17T15:49:48Z

nmrpy/data_objects.py

            size = len(data)
            ph = numpy.exp(1.0j*(p0+(p1*numpy.arange(size)/size)))
-            return ph*data
+            return ph*data, p0, p1


_ps() converts p0/p1 from degrees to radians, and now returns the radian values, which are then propagated to callers and stored in the data model. This changes external behavior (previously phase angles were effectively in degrees everywhere). Keep returning degrees (e.g., return the original degree inputs) or return both but ensure the values stored/exposed match the documented units (the UI output currently says degrees).

Copilot · 2026-02-17T15:49:51Z

specifications/datamodel_schema.md

+    NMRpy "1" <|-- "1" Experiment
+    Experiment "1" <|-- "*" FIDObject
+    FIDObject "1" <|-- "1" Parameters
+    FIDObject "1" <|-- "1" ProcessingSteps
+    FIDObject "1" <|-- "*" Peak
+    Peak "1" <|-- "1" PeakRange


In Mermaid class diagrams, <|-- denotes inheritance, but these relationships read like composition/association (NMRpy has an Experiment, Experiment has many FIDObject, etc.). Use association/composition arrows (e.g., o-- / *-- / -->) to reflect containment rather than subclassing.

Suggested change

NMRpy "1" <|-- "1" Experiment

Experiment "1" <|-- "*" FIDObject

FIDObject "1" <|-- "1" Parameters

FIDObject "1" <|-- "1" ProcessingSteps

FIDObject "1" <|-- "*" Peak

Peak "1" <|-- "1" PeakRange

NMRpy "1" *-- "1" Experiment

Experiment "1" *-- "*" FIDObject

FIDObject "1" *-- "1" Parameters

FIDObject "1" *-- "1" ProcessingSteps

FIDObject "1" *-- "*" Peak

Peak "1" *-- "1" PeakRange

Copilot · 2026-02-17T15:49:51Z

nmrpy/tests/nmrpy_tests.py

+        if (pyenzyme is None):
+            raise RuntimeError(


Raising RuntimeError in setUp() will fail the test run rather than cleanly skipping optional-dependency tests. Prefer self.skipTest(...) (or @unittest.skipUnless) so CI/users without the extra can still run the rest of the suite.

Suggested change

if (pyenzyme is None):

raise RuntimeError(

if pyenzyme is None:

self.skipTest(

Copilot · 2026-02-17T15:49:52Z

setup.py

 NMRPy is developed by Johann Eicher and Johann Rohwer from the Laboratory for
 Molecular Systems Biology, Dept. of Biochemistry, Stellenbosch University, 
-South Africa.
+South Africa, as well as Torsten Giess from the Insitute of Biochemistry and


Correct spelling: 'Insitute' → 'Institute'.

Suggested change

South Africa, as well as Torsten Giess from the Insitute of Biochemistry and

South Africa, as well as Torsten Giess from the Institute of Biochemistry and

Copilot · 2026-02-17T15:49:52Z

nmrpy/data_objects.py

+        )

-    def save_to_file(self, filename=None, overwrite=False):
+    def save_to_file(self, filename=None, overwrite=False, keep_data_model=False, keep_enzymeml=True):


The docstring says keep_data_model defaults to True, but the function signature sets keep_data_model=False. Update the docstring or the default to match actual behavior.

Copilot · 2026-02-17T15:49:52Z

nmrpy/data_objects.py


        :keyword overwrite: if True, overwrite existing file

+        :keyword keep_data_model: if True, keep the NMRpy data model (default is True)


The docstring says keep_data_model defaults to True, but the function signature sets keep_data_model=False. Update the docstring or the default to match actual behavior.

Suggested change

:keyword keep_data_model: if True, keep the NMRpy data model (default is True)

:keyword keep_data_model: if True, keep the NMRpy data model (default is False)

torogi94 added the enhancement label Jan 20, 2025

torogi94 requested a review from jmrohwer January 20, 2025 15:13

torogi94 assigned jmrohwer Jan 20, 2025

torogi94 force-pushed the data-model-refactor branch from b9db28b to 8182bfc Compare November 11, 2025 09:54

torogi94 and others added 20 commits November 13, 2025 12:49

Initial commit

c00b3d0

Minor changes

53ebb27

Implement IdentityAssigner for one FID object

85d3a4e

Update generate_api.yaml

97df96f

Update generate_api.yaml

29606d3

Update generate_api.yaml

1583813

Update generate_api.yaml

a87f2f6

Update generate_api.yaml

f164b24

Implement IdentityAssigner for entire FIDArray

188c365

Update data model

0bfbfe4

Minor bug fix

cb0def5

Add interface with EnzymeML

2feaa3d

Update IdentityAssigner

9f7fd0c

Update generate_api.yaml

aaea4f9

Update generate_api.yaml

c1bec90

Update generate_api.yaml

ce11a73

API update

030dcc5

Remove Citation info from data model

41c363e

API update

ecbaea2

Remove depricated data model

6b3e30a

torogi94 and others added 25 commits November 13, 2025 12:50

Fix recurring typo in pyenzyme import error

7b5c86f

Remove deprecated data model linking

fd90839

Add None check for new FidArray properties

dfbb0aa

- Initialisation of enzymeml_document and concentrations properties of FidArray class with initial None value led to Error. Check for None added to fix this issue. - Add correct optional dependency name to setup.py

Update requirements.txt

7e1b192

Due to an issue with md-models, pydantic>=2.10.0 is currently causing errors. In the meantime, a version restraint has been added to the requirements.txt

Update data_objects.py

1b0349d

Fix type checking bug in enzymeml_document property of FidArray class.

fix: Fix Fid.baseline_correct() and FidArray.baseline_correct_fids()

f6659b4

update ruff.toml

d674e41

fix data_model.setter ; formatting fixes

e0a724c

Fix FidArray.save_to_file() when deleting data model

651684c

rework calibrate() widget not to make use of asyncio.Future()

38072de

revert @data_model.setter changes as data_model is now deleted during…

c851b60

… save, not set to None

fix pyenzyme imports, update dependencies

e987ad3

Upgrade to pyenzyme v2

b28d5e6

Add t0 handling (#18)

3dc3d12

- Added T0Tab and T0Logic classes to utils.py - Added T0Adder widget class to plotting.py - Added add_t0_to_enzymeml() method that can be used interactively with a Jupyter widget using gui=True or script-like with gui=False

Fix copy() error in plotting.py

6cf0830

Add explicit super init to FidArray

4f48277

Update range handling in Fid (#20)

d796f8d

Changed behaviour of Fid._setup_peak_objects(). Multiple peaks per range are handled properly now by using Fid._grouped_peaklist instead of Fid.peaks. Also refactored the method to make it more robust and readable overall.

Add attribute checks for data model existence

80e4e12

Fix faulty argument to T0Adder widget

c3dca2d

Add data model test suites

7a3e67d

Add data model property unit tests + fixes

2309dc2

Fix parameter mapping to data model

9f55722

- Added NMR parameters to fid_object setter - Fixed p0 and p1 phasing parameter assignment

torogi94 force-pushed the data-model-refactor branch from 8182bfc to 9f55722 Compare November 13, 2025 11:51

Add proper single NMR in array edge case handling

ccb2087

Copilot AI review requested due to automatic review settings February 17, 2026 15:37

Copilot started reviewing on behalf of torogi94 February 17, 2026 15:43 View session

Copilot AI reviewed Feb 17, 2026

View reviewed changes

		from typing import Mapping, Optional

		from pyenzyme.versions.v2 import Protein

-            self.__fid_object.raw_data = [str(datum) for datum in self.raw_data]
-            self.__fid_object.processed_data = [float(datum) for datum in self.data]
-        except AttributeError:
+            # Always serialise raw_data as strings
+            self.__fid_object.raw_data = [str(datum) for datum in self.raw_data]
+            # Serialise processed_data, handling complex or otherwise non-floatable values gracefully
+            processed_data = []
+            for datum in self.data:
+                try:
+                    processed_data.append(float(datum))
+                except (TypeError, ValueError):
+                    # Fallback for complex or non-scalar values: store string representation
+                    processed_data.append(str(datum))
+            self.__fid_object.processed_data = processed_data
+        except (AttributeError, TypeError):

	South Africa, as well as Torsten Giess from the Insitute of Biochemistry and
	South Africa, as well as Torsten Giess from the Institute of Biochemistry and


		:keyword overwrite: if True, overwrite existing file

		:keyword keep_data_model: if True, keep the NMRpy data model (default is True)

Conversation

torogi94 commented Jan 20, 2025

Uh oh!

jmrohwer commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

torogi94 commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmrohwer commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

torogi94 commented Jan 29, 2025

Uh oh!

torogi94 commented May 5, 2025

Uh oh!

torogi94 commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

jmrohwer commented Jan 20, 2025 •

edited

Loading

torogi94 commented Jan 23, 2025 •

edited

Loading

jmrohwer commented Jan 24, 2025 •

edited

Loading

torogi94 commented Aug 27, 2025 •

edited

Loading